Skip to content

Commit

Permalink
Fix UTF-16 result handling for efibootmgr
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@juliakreger
juliakreger committed Apr 17, 2023
1 parent 0304c73 commit 76accfb
Show file tree
Hide file tree
Showing 4 changed files with 91 additions and 63 deletions.
15 changes: 11 additions & 4 deletions ironic_python_agent/efi_utils.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -275,8 +275,15 @@ def get_boot_records():
:return: an iterator yielding pairs (boot number, boot record).
"""
efi_output = utils.execute('efibootmgr', '-v')
for line in efi_output[0].split('\n'):
# Invokes binary=True so we get a bytestream back.
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.
cmd_output = efi_output[0].decode('utf-16', errors='ignore')
for line in cmd_output.split('\n'):
match = _ENTRY_LABEL.match(line)
if match is not None:
yield (match[1], match[2])
Expand All @@ -293,15 +300,15 @@ def add_boot_record(device, efi_partition, loader, label):
# https://linux.die.net/man/8/efibootmgr
utils.execute('efibootmgr', '-v', '-c', '-d', device,
'-p', str(efi_partition), '-w', '-L', label,
'-l', loader)
'-l', loader, binary=True)


def remove_boot_record(boot_num):
"""Remove an EFI boot record with efibootmgr.
:param boot_num: the number of the boot record
"""
utils.execute('efibootmgr', '-b', boot_num, '-B')
utils.execute('efibootmgr', '-b', boot_num, '-B', binary=True)


def _run_efibootmgr(valid_efi_bootloaders, device, efi_partition,
Expand Down
45 changes: 25 additions & 20 deletions ironic_python_agent/tests/unit/extensions/test_image.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@
from ironic_python_agent.tests.unit.samples import hardware_samples as hws


EFI_RESULT = ''.encode('utf-16')


@mock.patch.object(hardware, 'dispatch_to_managers', autospec=True)
@mock.patch.object(ilib_utils, 'execute', autospec=True)
@mock.patch.object(tempfile, 'mkdtemp', lambda *_: '/tmp/fake-dir')
Expand Down Expand Up @@ -231,7 +234,7 @@ def test__uefi_bootloader_given_partition(

mock_execute.side_effect = iter([('', ''), ('', ''),
('', ''), ('', ''),
('', ''), ('', ''),
(EFI_RESULT, ''), (EFI_RESULT, ''),
('', ''), ('', '')])

expected = [mock.call('efibootmgr', '--version'),
Expand All @@ -240,11 +243,11 @@ def test__uefi_bootloader_given_partition(
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',
'\\EFI\\BOOT\\BOOTX64.EFI'),
'\\EFI\\BOOT\\BOOTX64.EFI', binary=True),
mock.call('umount', self.fake_dir + '/boot/efi',
attempts=3, delay_on_retry=True),
mock.call('sync')]
Expand Down Expand Up @@ -279,7 +282,7 @@ def test__uefi_bootloader_find_partition(
mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI']
mock_execute.side_effect = iter([('', ''), ('', ''),
('', ''), ('', ''),
('', ''), ('', ''),
(EFI_RESULT, ''), (EFI_RESULT, ''),
('', ''), ('', '')])

expected = [mock.call('efibootmgr', '--version'),
Expand All @@ -288,11 +291,11 @@ def test__uefi_bootloader_find_partition(
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',
'\\EFI\\BOOT\\BOOTX64.EFI'),
'\\EFI\\BOOT\\BOOTX64.EFI', binary=True),
mock.call('umount', self.fake_dir + '/boot/efi',
attempts=3, delay_on_retry=True),
mock.call('sync')]
Expand Down Expand Up @@ -333,10 +336,11 @@ def test__uefi_bootloader_with_entry_removal(
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'),
Expand All @@ -345,12 +349,12 @@ def test__uefi_bootloader_with_entry_removal(
mock.call('udevadm', 'settle'),
mock.call('mount', self.fake_efi_system_part,
self.fake_dir + '/boot/efi'),
mock.call('efibootmgr', '-v'),
mock.call('efibootmgr', '-b', '0000', '-B'),
mock.call('efibootmgr', '-v', binary=True),
mock.call('efibootmgr', '-b', '0000', '-B', binary=True),
mock.call('efibootmgr', '-v', '-c', '-d', self.fake_dev,
'-p', '1', '-w',
'-L', 'ironic1', '-l',
'\\EFI\\BOOT\\BOOTX64.EFI'),
'\\EFI\\BOOT\\BOOTX64.EFI', binary=True),
mock.call('umount', self.fake_dir + '/boot/efi',
attempts=3, delay_on_retry=True),
mock.call('sync')]
Expand Down Expand Up @@ -396,6 +400,7 @@ def test__uefi_bootloader_with_entry_removal_lenovo(
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, ''), ('', ''),
Expand All @@ -407,13 +412,13 @@ def test__uefi_bootloader_with_entry_removal_lenovo(
mock.call('udevadm', 'settle'),
mock.call('mount', self.fake_efi_system_part,
self.fake_dir + '/boot/efi'),
mock.call('efibootmgr', '-v'),
mock.call('efibootmgr', '-b', '0000', '-B'),
mock.call('efibootmgr', '-b', '0004', '-B'),
mock.call('efibootmgr', '-v', binary=True),
mock.call('efibootmgr', '-b', '0000', '-B', binary=True),
mock.call('efibootmgr', '-b', '0004', '-B', binary=True),
mock.call('efibootmgr', '-v', '-c', '-d', self.fake_dev,
'-p', '1', '-w',
'-L', 'ironic1', '-l',
'\\EFI\\BOOT\\BOOTX64.EFI'),
'\\EFI\\BOOT\\BOOTX64.EFI', binary=True),
mock.call('umount', self.fake_dir + '/boot/efi',
attempts=3, delay_on_retry=True),
mock.call('sync')]
Expand Down Expand Up @@ -450,8 +455,8 @@ def test__add_multi_bootloaders(

mock_execute.side_effect = iter([('', ''), ('', ''),
('', ''), ('', ''),
('', ''), ('', ''),
('', ''), ('', ''),
(EFI_RESULT, ''), (EFI_RESULT, ''),
(EFI_RESULT, ''), ('', ''),
('', '')])

expected = [mock.call('efibootmgr', '--version'),
Expand All @@ -460,15 +465,15 @@ def test__add_multi_bootloaders(
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',
'\\EFI\\BOOT\\BOOTX64.EFI'),
'\\EFI\\BOOT\\BOOTX64.EFI', binary=True),
mock.call('efibootmgr', '-v', '-c', '-d', self.fake_dev,
'-p', '1', '-w',
'-L', 'ironic2', '-l',
'\\WINDOWS\\system32\\winload.efi'),
'\\WINDOWS\\system32\\winload.efi', binary=True),
mock.call('umount', self.fake_dir + '/boot/efi',
attempts=3, delay_on_retry=True),
mock.call('sync')]
Expand Down
Loading

0 comments on commit 76accfb

Please sign in to comment.