Skip to content

Commit e1813e1

Browse files
juliakregerjuliakreger
committed
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. NOTE: One of the unit tests from the stable/xena backport were removed, as software raid was still in-flight at the end of Wallaby. Closes-Bug: 2015602 Change-Id: I006535bf124379ed65443c7b283bc99ecc95568b (cherry picked from commit 76accfb) (cherry picked from commit 9f84c8b) (cherry picked from commit d77424d)
1 parent 9619ceb commit e1813e1

File tree

3 files changed

+52
-27
lines changed

3 files changed

+52
-27
lines changed

‎ironic_python_agent/extensions/image.py

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -275,7 +275,15 @@ def _run_efibootmgr(valid_efi_bootloaders, device, efi_partition,
275275

276276
# Before updating let's get information about the bootorder
277277
LOG.debug("Getting information about boot order.")
278-
original_efi_output = utils.execute('efibootmgr', '-v')
278+
# Invokes binary=True so we get a bytestream back.
279+
original_efi_output = utils.execute('efibootmgr', '-v', binary=True)
280+
# Bytes must be decoded before regex can be run and
281+
# matching to work as intended.
282+
# Also ignore errors on decoding, as we can basically get
283+
# garbage out of the nvram record, this way we don't fail
284+
# hard on unrelated records.
285+
original_efi_output = original_efi_output[0].decode(
286+
'utf-16', errors='ignore')
279287
# NOTE(TheJulia): regex used to identify entries in the efibootmgr
280288
# output on stdout.
281289
entry_label = re.compile(r'Boot([0-9a-f-A-F]+)\*?\s(.*).*$')
@@ -304,7 +312,7 @@ def _run_efibootmgr(valid_efi_bootloaders, device, efi_partition,
304312
label = 'ironic' + str(label_id)
305313

306314
# Iterate through standard out, and look for duplicates
307-
for line in original_efi_output[0].split('\n'):
315+
for line in original_efi_output.split('\n'):
308316
match = entry_label.match(line)
309317
# Look for the base label in the string if a line match
310318
# occurs, so we can identify if we need to eliminate the

‎ironic_python_agent/tests/unit/extensions/test_image.py

Lines changed: 33 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,9 @@
3030
from ironic_python_agent import utils
3131

3232

33+
EFI_RESULT = ''.encode('utf-16')
34+
35+
3336
@mock.patch.object(hardware, 'dispatch_to_managers', autospec=True)
3437
@mock.patch.object(utils, 'execute', autospec=True)
3538
@mock.patch.object(tempfile, 'mkdtemp', lambda *_: '/tmp/fake-dir')
@@ -251,7 +254,7 @@ def test__uefi_bootloader_given_partition(
251254

252255
mock_execute.side_effect = iter([('', ''), ('', ''),
253256
('', ''), ('', ''),
254-
('', ''), ('', ''),
257+
(EFI_RESULT, ''), (EFI_RESULT, ''),
255258
('', ''), ('', '')])
256259

257260
expected = [mock.call('efibootmgr', '--version'),
@@ -260,7 +263,7 @@ def test__uefi_bootloader_given_partition(
260263
mock.call('udevadm', 'settle'),
261264
mock.call('mount', self.fake_efi_system_part,
262265
self.fake_dir + '/boot/efi'),
263-
mock.call('efibootmgr', '-v'),
266+
mock.call('efibootmgr', '-v', binary=True),
264267
mock.call('efibootmgr', '-v', '-c', '-d', self.fake_dev,
265268
'-p', '1', '-w',
266269
'-L', 'ironic1', '-l',
@@ -299,7 +302,7 @@ def test__uefi_bootloader_find_partition(
299302
mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI']
300303
mock_execute.side_effect = iter([('', ''), ('', ''),
301304
('', ''), ('', ''),
302-
('', ''), ('', ''),
305+
(EFI_RESULT, ''), (EFI_RESULT, ''),
303306
('', ''), ('', '')])
304307

305308
expected = [mock.call('efibootmgr', '--version'),
@@ -308,7 +311,7 @@ def test__uefi_bootloader_find_partition(
308311
mock.call('udevadm', 'settle'),
309312
mock.call('mount', self.fake_efi_system_part,
310313
self.fake_dir + '/boot/efi'),
311-
mock.call('efibootmgr', '-v'),
314+
mock.call('efibootmgr', '-v', binary=True),
312315
mock.call('efibootmgr', '-v', '-c', '-d', self.fake_dev,
313316
'-p', '1', '-w',
314317
'-L', 'ironic1', '-l',
@@ -353,10 +356,11 @@ def test__uefi_bootloader_with_entry_removal(
353356
Boot0001 ironic2 HD(1,GPT,4f3c6294-bf9b-4208-9808-111111111112)File(\EFI\Boot\BOOTX64.EFI)
354357
Boot0002 VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51)
355358
""" # noqa This is a giant literal string for testing.
359+
stdout_msg = stdout_msg.encode('utf-16')
356360
mock_execute.side_effect = iter([('', ''), ('', ''),
357361
('', ''), ('', ''),
358-
(stdout_msg, ''), ('', ''),
359-
('', ''), ('', ''),
362+
(stdout_msg, ''), (EFI_RESULT, ''),
363+
(EFI_RESULT, ''), (EFI_RESULT, ''),
360364
('', ''), ('', '')])
361365

362366
expected = [mock.call('efibootmgr', '--version'),
@@ -365,7 +369,7 @@ def test__uefi_bootloader_with_entry_removal(
365369
mock.call('udevadm', 'settle'),
366370
mock.call('mount', self.fake_efi_system_part,
367371
self.fake_dir + '/boot/efi'),
368-
mock.call('efibootmgr', '-v'),
372+
mock.call('efibootmgr', '-v', binary=True),
369373
mock.call('efibootmgr', '-b', '0000', '-B'),
370374
mock.call('efibootmgr', '-v', '-c', '-d', self.fake_dev,
371375
'-p', '1', '-w',
@@ -416,6 +420,7 @@ def test__uefi_bootloader_with_entry_removal_lenovo(
416420
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. .-.
417421
Boot0004* ironic1 HD(1,GPT,55db8d03-c8f6-4a5b-9155-790dddc348fa,0x800,0x64000)/File(\EFI\boot\shimx64.efi)
418422
""" # noqa This is a giant literal string for testing.
423+
stdout_msg = stdout_msg.encode('utf-16')
419424
mock_execute.side_effect = iter([('', ''), ('', ''),
420425
('', ''), ('', ''),
421426
(stdout_msg, ''), ('', ''),
@@ -427,7 +432,7 @@ def test__uefi_bootloader_with_entry_removal_lenovo(
427432
mock.call('udevadm', 'settle'),
428433
mock.call('mount', self.fake_efi_system_part,
429434
self.fake_dir + '/boot/efi'),
430-
mock.call('efibootmgr', '-v'),
435+
mock.call('efibootmgr', '-v', binary=True),
431436
mock.call('efibootmgr', '-b', '0000', '-B'),
432437
mock.call('efibootmgr', '-b', '0004', '-B'),
433438
mock.call('efibootmgr', '-v', '-c', '-d', self.fake_dev,
@@ -470,8 +475,8 @@ def test__add_multi_bootloaders(
470475

471476
mock_execute.side_effect = iter([('', ''), ('', ''),
472477
('', ''), ('', ''),
473-
('', ''), ('', ''),
474-
('', ''), ('', ''),
478+
(EFI_RESULT, ''), (EFI_RESULT, ''),
479+
(EFI_RESULT, ''), ('', ''),
475480
('', '')])
476481

477482
expected = [mock.call('efibootmgr', '--version'),
@@ -480,7 +485,7 @@ def test__add_multi_bootloaders(
480485
mock.call('udevadm', 'settle'),
481486
mock.call('mount', self.fake_efi_system_part,
482487
self.fake_dir + '/boot/efi'),
483-
mock.call('efibootmgr', '-v'),
488+
mock.call('efibootmgr', '-v', binary=True),
484489
mock.call('efibootmgr', '-v', '-c', '-d', self.fake_dev,
485490
'-p', '1', '-w',
486491
'-L', 'ironic1', '-l',
@@ -2379,7 +2384,7 @@ def test__manage_uefi(self, mkdir_mock, mock_utils_efi_part,
23792384
mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI']
23802385

23812386
mock_execute.side_effect = iter([('', ''), ('', ''),
2382-
('', ''), ('', ''),
2387+
('', ''), (b'', ''),
23832388
('', ''), ('', ''),
23842389
('', '')])
23852390

@@ -2388,7 +2393,7 @@ def test__manage_uefi(self, mkdir_mock, mock_utils_efi_part,
23882393
mock.call('udevadm', 'settle'),
23892394
mock.call('mount', self.fake_efi_system_part,
23902395
self.fake_dir + '/boot/efi'),
2391-
mock.call('efibootmgr', '-v'),
2396+
mock.call('efibootmgr', '-v', binary=True),
23922397
mock.call('efibootmgr', '-v', '-c', '-d', self.fake_dev,
23932398
'-p', '1', '-w',
23942399
'-L', 'ironic1', '-l',
@@ -2432,18 +2437,19 @@ def test__manage_uefi_found_csv(self, mkdir_mock, mock_utils_efi_part,
24322437
Boot0002: VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51)
24332438
""" # noqa This is a giant literal string for testing.
24342439

2435-
mock_execute.side_effect = iter([('', ''), ('', ''),
2436-
('', ''), (dupe_entry, ''),
2437-
('', ''), ('', ''),
2438-
('', ''), ('', ''),
2439-
('', '')])
2440+
mock_execute.side_effect = iter([
2441+
('', ''), ('', ''),
2442+
('', ''), (dupe_entry.encode('utf-16'), ''),
2443+
('', ''), ('', ''),
2444+
('', ''), ('', ''),
2445+
('', '')])
24402446

24412447
expected = [mock.call('partx', '-a', '/dev/fake', attempts=3,
24422448
delay_on_retry=True),
24432449
mock.call('udevadm', 'settle'),
24442450
mock.call('mount', self.fake_efi_system_part,
24452451
self.fake_dir + '/boot/efi'),
2446-
mock.call('efibootmgr', '-v'),
2452+
mock.call('efibootmgr', '-v', binary=True),
24472453
mock.call('efibootmgr', '-b', '0000', '-B'),
24482454
mock.call('efibootmgr', '-b', '0001', '-B'),
24492455
mock.call('efibootmgr', '-v', '-c', '-d', self.fake_dev,
@@ -2475,7 +2481,7 @@ def test__manage_uefi_nvme_device(self, mkdir_mock, mock_utils_efi_part,
24752481
mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI']
24762482

24772483
mock_execute.side_effect = iter([('', ''), ('', ''),
2478-
('', ''), ('', ''),
2484+
('', ''), (b'', ''),
24792485
('', ''), ('', ''),
24802486
('', '')])
24812487

@@ -2484,7 +2490,7 @@ def test__manage_uefi_nvme_device(self, mkdir_mock, mock_utils_efi_part,
24842490
mock.call('udevadm', 'settle'),
24852491
mock.call('mount', '/dev/fakenvme0p1',
24862492
self.fake_dir + '/boot/efi'),
2487-
mock.call('efibootmgr', '-v'),
2493+
mock.call('efibootmgr', '-v', binary=True),
24882494
mock.call('efibootmgr', '-v', '-c', '-d', '/dev/fakenvme0',
24892495
'-p', '1', '-w',
24902496
'-L', 'ironic1', '-l',
@@ -2513,7 +2519,7 @@ def test__manage_uefi_wholedisk(
25132519
mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI']
25142520

25152521
mock_execute.side_effect = iter([('', ''), ('', ''),
2516-
('', ''), ('', ''),
2522+
('', ''), (b'', ''),
25172523
('', ''), ('', ''),
25182524
('', '')])
25192525

@@ -2522,7 +2528,7 @@ def test__manage_uefi_wholedisk(
25222528
mock.call('udevadm', 'settle'),
25232529
mock.call('mount', self.fake_efi_system_part,
25242530
self.fake_dir + '/boot/efi'),
2525-
mock.call('efibootmgr', '-v'),
2531+
mock.call('efibootmgr', '-v', binary=True),
25262532
mock.call('efibootmgr', '-v', '-c', '-d', self.fake_dev,
25272533
'-p', '1', '-w',
25282534
'-L', 'ironic1', '-l',
@@ -2625,12 +2631,14 @@ def test__run_efibootmgr_no_bootloaders(self, mock_execute, mock_dispatch):
26252631
mock_execute.assert_has_calls(expected)
26262632

26272633
def test__run_efibootmgr(self, mock_execute, mock_dispatch):
2628-
mock_execute.return_value = ('', '')
2634+
mock_execute.side_effect = [
2635+
(b'', ''),
2636+
('', '')]
26292637
result = image._run_efibootmgr(['EFI/BOOT/BOOTX64.EFI'],
26302638
self.fake_dev,
26312639
self.fake_efi_system_part,
26322640
self.fake_dir)
2633-
expected = [mock.call('efibootmgr', '-v'),
2641+
expected = [mock.call('efibootmgr', '-v', binary=True),
26342642
mock.call('efibootmgr', '-v', '-c', '-d', self.fake_dev,
26352643
'-p', self.fake_efi_system_part, '-w',
26362644
'-L', 'ironic1', '-l',

‎releasenotes/notes/fixes-efibootmgr-character-encoding-19e531ba694824c1.yaml

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
---
2+
fixes:
3+
- |
4+
Fixes UEFI NVRAM record handling with efibootmgr so we can accept and
5+
handle UTF-16 encoded data which is to be expected in UEFI NVRAM as
6+
the records are UTF-16 encoded.
7+
- |
8+
Fixes handling of UEFI NVRAM records to allow for unexpected characters
9+
in the response, so it is non-fatal to Ironic.

0 commit comments

Comments
 (0)