openstack/ironic-python-agent
Code Issues Proposed changes

Fix UTF-16 result handling for efibootmgr

Browse Source 
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
This commit is contained in:
Julia Kreger 2023-04-07 07:32:44 -07:00
parent 0304c73c0e
commit 76accfb880
4 changed files with 91 additions and 63 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
ironic_python_agent/efi_utils.py
View File

@ -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])
@ -293,7 +300,7 @@ 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):
@ -301,7 +308,7 @@ def remove_boot_record(boot_num):
: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,

45
ironic_python_agent/tests/unit/extensions/test_image.py
View File

@ -32,6 +32,9 @@ from ironic_python_agent.tests.unit import base
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')
@ -231,7 +234,7 @@ class TestImageExtension(base.IronicAgentTest):
mock_execute.side_effect = iter([('', ''), ('', ''),
('', ''), ('', ''),
('', ''), ('', ''),
(EFI_RESULT, ''), (EFI_RESULT, ''),
('', ''), ('', '')])
expected = [mock.call('efibootmgr', '--version'),
@ -240,11 +243,11 @@ 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',
'\\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')]
@ -279,7 +282,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'),
@ -288,11 +291,11 @@ 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',
'\\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')]
@ -333,10 +336,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'),
@ -345,12 +349,12 @@ 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', '-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')]
@ -396,6 +400,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, ''), ('', ''),
@ -407,13 +412,13 @@ 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', '-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')]
@ -450,8 +455,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'),
@ -460,15 +465,15 @@ 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',
'\\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')]

85
ironic_python_agent/tests/unit/test_efi_utils.py
View File

@ -28,6 +28,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(os, 'walk', autospec=True)
@mock.patch.object(os, 'access', autospec=False)
class TestGetEfiBootloaders(base.IronicAgentTest):
@ -115,16 +118,16 @@ class TestRunEfiBootmgr(base.IronicAgentTest):
mock_execute.assert_has_calls(expected)
def test__run_efibootmgr(self, mock_execute):
mock_execute.return_value = ('', '')
mock_execute.return_value = (''.encode('utf-16'), '')
result = efi_utils._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',
'\\EFI\\BOOT\\BOOTX64.EFI')]
'\\EFI\\BOOT\\BOOTX64.EFI', binary=True)]
self.assertIsNone(result)
mock_execute.assert_has_calls(expected)
@ -176,19 +179,18 @@ class TestManageUefi(base.IronicAgentTest):
mock_is_md_device.return_value = False
mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI']
mock_execute.side_effect = iter([('', ''), ('', ''),
('', ''), ('', ''),
mock_execute.side_effect = iter([('', ''), (EFI_RESULT, ''),
(EFI_RESULT, ''), ('', ''),
('', ''), ('', ''),
('', '')])
expected = [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')]
@ -219,30 +221,34 @@ class TestManageUefi(base.IronicAgentTest):
# at the start of the file, where as Red Hat *does*
csv_file_data = u'shimx64.efi,Vendor String,,Grub2MadeUSDoThis\n'
# This test also handles deleting a pre-existing matching vendor
# string in advance.
# string in advance. This string also includes a UTF16 character
# *on* purpose, to force proper decoding to be tested and garbage
# characters which can be found in OVMF test VM NVRAM records.
dupe_entry = """
BootCurrent: 0001
Timeout: 0 seconds
BootOrder: 0000,00001
Boot0000* Vendor String HD(1,GPT,4f3c6294-bf9b-4208-9808-be45dfc34b5c)File(\EFI\Boot\BOOTX64.EFI)
Boot0001 Vendor String HD(2,GPT,4f3c6294-bf9b-4208-9808-be45dfc34b5c)File(\EFI\Boot\BOOTX64.EFI)
Boot0002: VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51)
Boot0000 UTF16ÿ HD(1,GPT,4f3c6294-bf9b-4208-9808-be45dfc34b5c)File(\EFI\Boot\BOOTX64.EFI)
Boot0001* Vendor String HD(1,GPT,4f3c6294-bf9b-4208-9808-be45dfc34b5c)File(\EFI\Boot\BOOTX64.EFI)
Boot0002 Vendor String HD(2,GPT,4f3c6294-bf9b-4208-9808-be45dfc34b5c)File(\EFI\Boot\BOOTX64.EFI)
Boot0003: VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51)N.....YM....R,Y.
""" # noqa This is a giant literal string for testing.
mock_execute.side_effect = iter([('', ''), (dupe_entry, ''),
dupe_entry = dupe_entry.encode('utf-16')
mock_execute.side_effect = iter([('', ''),
(dupe_entry, ''),
('', ''), ('', ''),
('', ''), ('', ''),
('', '')])
expected = [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', '0001', '-B'),
mock.call('efibootmgr', '-v', binary=True),
mock.call('efibootmgr', '-b', '0001', '-B', binary=True),
mock.call('efibootmgr', '-b', '0002', '-B', binary=True),
mock.call('efibootmgr', '-v', '-c', '-d', self.fake_dev,
'-p', '1', '-w',
'-L', 'Vendor String', '-l',
'\\EFI\\vendor\\shimx64.efi'),
'\\EFI\\vendor\\shimx64.efi', binary=True),
mock.call('umount', self.fake_dir + '/boot/efi',
attempts=3, delay_on_retry=True),
mock.call('sync')]
@ -266,19 +272,18 @@ Boot0002: VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51)
mock_is_md_device.return_value = False
mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI']
mock_execute.side_effect = iter([('', ''), ('', ''),
('', ''), ('', ''),
mock_execute.side_effect = iter([('', ''), (EFI_RESULT, ''),
(EFI_RESULT, ''), ('', ''),
('', ''), ('', ''),
('', '')])
expected = [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',
'\\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')]
@ -301,19 +306,18 @@ Boot0002: VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51)
mock_is_md_device.return_value = False
mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI']
mock_execute.side_effect = iter([('', ''), ('', ''),
('', ''), ('', ''),
mock_execute.side_effect = iter([('', ''), (EFI_RESULT, ''),
(EFI_RESULT, ''), ('', ''),
('', ''), ('', ''),
('', '')])
expected = [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')]
@ -347,11 +351,14 @@ Boot0002: VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51)
mock_get_component_devices.return_value = ['/dev/sda3', '/dev/sdb3']
mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI']
mock_execute.side_effect = iter([('', ''), ('', ''),
('', ''), ('', ''),
('', ''), ('', ''),
('', ''), ('', ''),
mock_execute.side_effect = iter([('', ''),
('', ''),
('', ''),
(EFI_RESULT, ''),
(EFI_RESULT, ''),
(EFI_RESULT, ''),
(EFI_RESULT, ''),
('', ''),
('', '')])
expected = [mock.call('mount', self.fake_efi_system_part,
@ -360,14 +367,14 @@ 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'),
'-l', '\\EFI\\BOOT\\BOOTX64.EFI', binary=True),
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'),
'-l', '\\EFI\\BOOT\\BOOTX64.EFI', binary=True),
mock.call('umount', self.fake_dir + '/boot/efi',
attempts=3, delay_on_retry=True),
mock.call('sync')]
@ -425,7 +432,7 @@ Boot0002: VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51)
expected = [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('umount', self.fake_dir + '/boot/efi',
attempts=3, delay_on_retry=True),
mock.call('sync')]
@ -462,7 +469,7 @@ Boot0002: VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51)
expected = [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('umount', self.fake_dir + '/boot/efi',
attempts=3, delay_on_retry=True)]

9
releasenotes/notes/fixes-efibootmgr-character-encoding-19e531ba694824c1.yaml Normal file
View File

@ -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.