From d77424d7315e24390d6b159eab8dd9b3d4c56942 Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Fri, 7 Apr 2023 07:32:44 -0700 Subject: Fix UTF-16 result handling for efibootmgr The tl;dr is that UEFI NVRAM is in encoded in UTF-16, and when we run the efibootmgr command, we can get unicode characters back. Except we previously were forcing everything to be treated as UTF-8 due to the way oslo.concurrency's processutils module works. This could be observed with UTF character 0x00FF which raises up a nice exception when we try to decode it. Anyhow! while fixing handling of this, we discovered we could get basically the cruft out of the NVRAM, by getting what was most likey a truncated string out of our own test VMs. As such, we need to also permit decoding to be tollerant of failures. This could be binary data or as simple as flipped bits which get interpretted invalid characters. As such, we have introduced such data into one of our tests involving UEFI record de-duplication. Closes-Bug: 2015602 Change-Id: I006535bf124379ed65443c7b283bc99ecc95568b (cherry picked from commit 76accfb880474445a5dcb07825889123b3dd0237) (cherry picked from commit 9f84c8b3d1fa0e08bf1f799f37a11698f8da07a4) --- ironic_python_agent/extensions/image.py | 12 +++- .../tests/unit/extensions/test_image.py | 66 ++++++++++++---------- ...ootmgr-character-encoding-19e531ba694824c1.yaml | 9 +++ 3 files changed, 56 insertions(+), 31 deletions(-) create mode 100644 releasenotes/notes/fixes-efibootmgr-character-encoding-19e531ba694824c1.yaml diff --git a/ironic_python_agent/extensions/image.py b/ironic_python_agent/extensions/image.py index d9141002..be2707ec 100644 --- a/ironic_python_agent/extensions/image.py +++ b/ironic_python_agent/extensions/image.py @@ -278,7 +278,15 @@ def _run_efibootmgr(valid_efi_bootloaders, device, efi_partition, # Before updating let's get information about the bootorder LOG.debug("Getting information about boot order.") - original_efi_output = utils.execute('efibootmgr', '-v') + # Invokes binary=True so we get a bytestream back. + original_efi_output = utils.execute('efibootmgr', '-v', binary=True) + # Bytes must be decoded before regex can be run and + # matching to work as intended. + # Also ignore errors on decoding, as we can basically get + # garbage out of the nvram record, this way we don't fail + # hard on unrelated records. + original_efi_output = original_efi_output[0].decode( + 'utf-16', errors='ignore') # NOTE(TheJulia): regex used to identify entries in the efibootmgr # output on stdout. entry_label = re.compile(r'Boot([0-9a-f-A-F]+)\*?\s(.*).*$') @@ -311,7 +319,7 @@ def _run_efibootmgr(valid_efi_bootloaders, device, efi_partition, label = label + " " + str(label_suffix) # Iterate through standard out, and look for duplicates - for line in original_efi_output[0].split('\n'): + for line in original_efi_output.split('\n'): match = entry_label.match(line) # Look for the base label in the string if a line match # occurs, so we can identify if we need to eliminate the diff --git a/ironic_python_agent/tests/unit/extensions/test_image.py b/ironic_python_agent/tests/unit/extensions/test_image.py index 0d2ba0d3..3115aee2 100644 --- a/ironic_python_agent/tests/unit/extensions/test_image.py +++ b/ironic_python_agent/tests/unit/extensions/test_image.py @@ -31,6 +31,9 @@ from ironic_python_agent.tests.unit.samples import hardware_samples from ironic_python_agent import utils +EFI_RESULT = ''.encode('utf-16') + + @mock.patch.object(hardware, 'dispatch_to_managers', autospec=True) @mock.patch.object(utils, 'execute', autospec=True) @mock.patch.object(tempfile, 'mkdtemp', lambda *_: '/tmp/fake-dir') @@ -230,7 +233,7 @@ class TestImageExtension(base.IronicAgentTest): mock_execute.side_effect = iter([('', ''), ('', ''), ('', ''), ('', ''), - ('', ''), ('', ''), + (EFI_RESULT, ''), (EFI_RESULT, ''), ('', ''), ('', '')]) expected = [mock.call('efibootmgr', '--version'), @@ -239,7 +242,7 @@ class TestImageExtension(base.IronicAgentTest): mock.call('udevadm', 'settle'), mock.call('mount', self.fake_efi_system_part, self.fake_dir + '/boot/efi'), - mock.call('efibootmgr', '-v'), + mock.call('efibootmgr', '-v', binary=True), mock.call('efibootmgr', '-v', '-c', '-d', self.fake_dev, '-p', '1', '-w', '-L', 'ironic1', '-l', @@ -277,7 +280,7 @@ class TestImageExtension(base.IronicAgentTest): mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI'] mock_execute.side_effect = iter([('', ''), ('', ''), ('', ''), ('', ''), - ('', ''), ('', ''), + (EFI_RESULT, ''), (EFI_RESULT, ''), ('', ''), ('', '')]) expected = [mock.call('efibootmgr', '--version'), @@ -286,7 +289,7 @@ class TestImageExtension(base.IronicAgentTest): mock.call('udevadm', 'settle'), mock.call('mount', self.fake_efi_system_part, self.fake_dir + '/boot/efi'), - mock.call('efibootmgr', '-v'), + mock.call('efibootmgr', '-v', binary=True), mock.call('efibootmgr', '-v', '-c', '-d', self.fake_dev, '-p', '1', '-w', '-L', 'ironic1', '-l', @@ -330,10 +333,11 @@ Boot0000 ironic1 HD(1,GPT,4f3c6294-bf9b-4208-9808-be45dfc34b5c)File(\EFI\Boot\BO Boot0001 ironic2 HD(1,GPT,4f3c6294-bf9b-4208-9808-111111111112)File(\EFI\Boot\BOOTX64.EFI) Boot0002 VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51) """ # noqa This is a giant literal string for testing. + stdout_msg = stdout_msg.encode('utf-16') mock_execute.side_effect = iter([('', ''), ('', ''), ('', ''), ('', ''), - (stdout_msg, ''), ('', ''), - ('', ''), ('', ''), + (stdout_msg, ''), (EFI_RESULT, ''), + (EFI_RESULT, ''), (EFI_RESULT, ''), ('', ''), ('', '')]) expected = [mock.call('efibootmgr', '--version'), @@ -342,7 +346,7 @@ Boot0002 VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51) mock.call('udevadm', 'settle'), mock.call('mount', self.fake_efi_system_part, self.fake_dir + '/boot/efi'), - mock.call('efibootmgr', '-v'), + mock.call('efibootmgr', '-v', binary=True), mock.call('efibootmgr', '-b', '0000', '-B'), mock.call('efibootmgr', '-v', '-c', '-d', self.fake_dev, '-p', '1', '-w', @@ -392,6 +396,7 @@ Boot0002* Hard Disk VenHw(1fad3248-0000-7950-2166-a1e506fdb83a,01000000)..GO Boot0003* Network VenHw(1fad3248-0000-7950-2166-a1e506fdb83a,05000000)..GO..NO............U.E.F.I.:. . . .S.L.O.T.2. .(.2.F./.0./.0.). .P.X.E. .I.P.4. . .Q.L.o.g.i.c. .Q.L.4.1.2.6.2. .P.C.I.e. .2.5.G.b. .2.-.P.o.r.t. .S.F.P.2.8. .E.t.h.e.r.n.e.t. .A.d.a.p.t.e.r. .-. .P.X.E........A....................%.4..Z...............................................................Gd-.;.A..MQ..L.P.X.E. .I.P.4. .Q.L.o.g.i.c. .Q.L.4.1.2.6.2. .P.C.I.e. .2.5.G.b. .2.-.P.o.r.t. .S.F.P.2.8. .E.t.h.e.r.n.e.t. .A.d.a.p.t.e.r. .-. .P.X.E.......BO..NO............U.E.F.I.:. . . .S.L.O.T.1. .(.3.0./.0./.0.). .P.X.E. .I.P.4. . .Q.L.o.g.i.c. .Q.L.4.1.2.6.2. .P.C.I.e. .2.5.G.b. .2.-.P.o.r.t. .S.F.P.2.8. .E.t.h.e.r.n.e.t. .A.d.a.p.t.e.r. .-. Boot0004* ironic1 HD(1,GPT,55db8d03-c8f6-4a5b-9155-790dddc348fa,0x800,0x64000)/File(\EFI\boot\shimx64.efi) """ # noqa This is a giant literal string for testing. + stdout_msg = stdout_msg.encode('utf-16') mock_execute.side_effect = iter([('', ''), ('', ''), ('', ''), ('', ''), (stdout_msg, ''), ('', ''), @@ -403,7 +408,7 @@ Boot0004* ironic1 HD(1,GPT,55db8d03-c8f6-4a5b-9155-790dddc348fa,0x800,0x640 mock.call('udevadm', 'settle'), mock.call('mount', self.fake_efi_system_part, self.fake_dir + '/boot/efi'), - mock.call('efibootmgr', '-v'), + mock.call('efibootmgr', '-v', binary=True), mock.call('efibootmgr', '-b', '0000', '-B'), mock.call('efibootmgr', '-b', '0004', '-B'), mock.call('efibootmgr', '-v', '-c', '-d', self.fake_dev, @@ -445,8 +450,8 @@ Boot0004* ironic1 HD(1,GPT,55db8d03-c8f6-4a5b-9155-790dddc348fa,0x800,0x640 mock_execute.side_effect = iter([('', ''), ('', ''), ('', ''), ('', ''), - ('', ''), ('', ''), - ('', ''), ('', ''), + (EFI_RESULT, ''), (EFI_RESULT, ''), + (EFI_RESULT, ''), ('', ''), ('', '')]) expected = [mock.call('efibootmgr', '--version'), @@ -455,7 +460,7 @@ Boot0004* ironic1 HD(1,GPT,55db8d03-c8f6-4a5b-9155-790dddc348fa,0x800,0x640 mock.call('udevadm', 'settle'), mock.call('mount', self.fake_efi_system_part, self.fake_dir + '/boot/efi'), - mock.call('efibootmgr', '-v'), + mock.call('efibootmgr', '-v', binary=True), mock.call('efibootmgr', '-v', '-c', '-d', self.fake_dev, '-p', '1', '-w', '-L', 'ironic1', '-l', @@ -2341,7 +2346,7 @@ Boot0004* ironic1 HD(1,GPT,55db8d03-c8f6-4a5b-9155-790dddc348fa,0x800,0x640 mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI'] mock_execute.side_effect = iter([('', ''), ('', ''), - ('', ''), ('', ''), + ('', ''), (b'', ''), ('', ''), ('', ''), ('', '')]) @@ -2350,7 +2355,7 @@ Boot0004* ironic1 HD(1,GPT,55db8d03-c8f6-4a5b-9155-790dddc348fa,0x800,0x640 mock.call('udevadm', 'settle'), mock.call('mount', self.fake_efi_system_part, self.fake_dir + '/boot/efi'), - mock.call('efibootmgr', '-v'), + mock.call('efibootmgr', '-v', binary=True), mock.call('efibootmgr', '-v', '-c', '-d', self.fake_dev, '-p', '1', '-w', '-L', 'ironic1', '-l', @@ -2397,18 +2402,19 @@ Boot0001 Vendor String HD(2,GPT,4f3c6294-bf9b-4208-9808-be45dfc34b5c)File(\EFI\B Boot0002: VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51) """ # noqa This is a giant literal string for testing. - mock_execute.side_effect = iter([('', ''), ('', ''), - ('', ''), (dupe_entry, ''), - ('', ''), ('', ''), - ('', ''), ('', ''), - ('', '')]) + mock_execute.side_effect = iter([ + ('', ''), ('', ''), + ('', ''), (dupe_entry.encode('utf-16'), ''), + ('', ''), ('', ''), + ('', ''), ('', ''), + ('', '')]) expected = [mock.call('partx', '-a', '/dev/fake', attempts=3, delay_on_retry=True), mock.call('udevadm', 'settle'), mock.call('mount', self.fake_efi_system_part, self.fake_dir + '/boot/efi'), - mock.call('efibootmgr', '-v'), + mock.call('efibootmgr', '-v', binary=True), mock.call('efibootmgr', '-b', '0000', '-B'), mock.call('efibootmgr', '-b', '0001', '-B'), mock.call('efibootmgr', '-v', '-c', '-d', self.fake_dev, @@ -2443,7 +2449,7 @@ Boot0002: VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51) mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI'] mock_execute.side_effect = iter([('', ''), ('', ''), - ('', ''), ('', ''), + ('', ''), (b'', ''), ('', ''), ('', ''), ('', '')]) @@ -2452,7 +2458,7 @@ Boot0002: VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51) mock.call('udevadm', 'settle'), mock.call('mount', '/dev/fakenvme0p1', self.fake_dir + '/boot/efi'), - mock.call('efibootmgr', '-v'), + mock.call('efibootmgr', '-v', binary=True), mock.call('efibootmgr', '-v', '-c', '-d', '/dev/fakenvme0', '-p', '1', '-w', '-L', 'ironic1', '-l', @@ -2484,7 +2490,7 @@ Boot0002: VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51) mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI'] mock_execute.side_effect = iter([('', ''), ('', ''), - ('', ''), ('', ''), + ('', ''), (b'', ''), ('', ''), ('', ''), ('', '')]) @@ -2493,7 +2499,7 @@ Boot0002: VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51) mock.call('udevadm', 'settle'), mock.call('mount', self.fake_efi_system_part, self.fake_dir + '/boot/efi'), - mock.call('efibootmgr', '-v'), + mock.call('efibootmgr', '-v', binary=True), mock.call('efibootmgr', '-v', '-c', '-d', self.fake_dev, '-p', '1', '-w', '-L', 'ironic1', '-l', @@ -2535,8 +2541,8 @@ Boot0002: VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51) mock_execute.side_effect = iter([('', ''), ('', ''), ('', ''), ('', ''), - ('', ''), ('', ''), - ('', ''), ('', ''), + ('', ''), (b'', ''), + ('', ''), (b'', ''), ('', ''), ('', ''), ('', '')]) @@ -2549,11 +2555,11 @@ Boot0002: VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51) attempts=3, delay_on_retry=True), mock.call('mount', self.fake_efi_system_part, self.fake_dir + '/boot/efi'), - mock.call('efibootmgr', '-v'), + mock.call('efibootmgr', '-v', binary=True), mock.call('efibootmgr', '-v', '-c', '-d', '/dev/sda3', '-p', '3', '-w', '-L', 'ironic1 (RAID, part0)', '-l', '\\EFI\\BOOT\\BOOTX64.EFI'), - mock.call('efibootmgr', '-v'), + mock.call('efibootmgr', '-v', binary=True), mock.call('efibootmgr', '-v', '-c', '-d', '/dev/sdb3', '-p', '3', '-w', '-L', 'ironic1 (RAID, part1)', '-l', '\\EFI\\BOOT\\BOOTX64.EFI'), @@ -2654,12 +2660,14 @@ Boot0002: VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51) mock_execute.assert_has_calls(expected) def test__run_efibootmgr(self, mock_execute, mock_dispatch): - mock_execute.return_value = ('', '') + mock_execute.side_effect = [ + (b'', ''), + ('', '')] result = image._run_efibootmgr(['EFI/BOOT/BOOTX64.EFI'], self.fake_dev, self.fake_efi_system_part, self.fake_dir) - expected = [mock.call('efibootmgr', '-v'), + expected = [mock.call('efibootmgr', '-v', binary=True), mock.call('efibootmgr', '-v', '-c', '-d', self.fake_dev, '-p', self.fake_efi_system_part, '-w', '-L', 'ironic1', '-l', diff --git a/releasenotes/notes/fixes-efibootmgr-character-encoding-19e531ba694824c1.yaml b/releasenotes/notes/fixes-efibootmgr-character-encoding-19e531ba694824c1.yaml new file mode 100644 index 00000000..b4c3270c --- /dev/null +++ b/releasenotes/notes/fixes-efibootmgr-character-encoding-19e531ba694824c1.yaml @@ -0,0 +1,9 @@ +--- +fixes: + - | + Fixes UEFI NVRAM record handling with efibootmgr so we can accept and + handle UTF-16 encoded data which is to be expected in UEFI NVRAM as + the records are UTF-16 encoded. + - | + Fixes handling of UEFI NVRAM records to allow for unexpected characters + in the response, so it is non-fatal to Ironic. -- cgit v1.2.1