From 215fecd4470e868e1bac9737417e166a7e10fb64 Mon Sep 17 00:00:00 2001 From: Steve Baker Date: Thu, 4 Apr 2024 10:42:55 +1300 Subject: [PATCH] Step to clean UEFI NVRAM entries Adds a deploy step ``clean_uefi_nvram`` to remove unrequired extra UEFI NVRAM boot entries. By default any entry matching ``HD`` as the root device, or with a ``shim`` or ``grub`` efi file in the path will be deleted, ensuring that disk based boot entries are removed before the new entry is created for the written image. The ``match_patterns`` parameter allows a list of regular expressions to be passed, where a case insensitive search in the device path will result in that entry being deleted. Closes-Bug: #2041901 Change-Id: I3559dc800fcdfb0322286eba30ce47041419b0c6 --- ironic_python_agent/efi_utils.py | 17 ++++++ ironic_python_agent/hardware.py | 60 +++++++++++++++++++ .../tests/unit/test_efi_utils.py | 49 +++++++++++++-- .../tests/unit/test_hardware.py | 44 ++++++++++++++ .../clean_uefi_nvram-554041f2e7b2d555.yaml | 10 ++++ 5 files changed, 176 insertions(+), 4 deletions(-) create mode 100644 releasenotes/notes/clean_uefi_nvram-554041f2e7b2d555.yaml diff --git a/ironic_python_agent/efi_utils.py b/ironic_python_agent/efi_utils.py index 5be8dbc43..a628e4604 100644 --- a/ironic_python_agent/efi_utils.py +++ b/ironic_python_agent/efi_utils.py @@ -314,6 +314,23 @@ def remove_boot_record(boot_num): utils.execute('efibootmgr', '-b', boot_num, '-B', binary=True) +def clean_boot_records(patterns): + """Remove EFI boot records matching regex patterns. + + :param match_patterns: A list of string regular expression patterns + where any matching entry will be deleted. + """ + + for boot_num, entry, _, path in get_boot_records(): + for pattern in patterns: + if pattern.search(path): + LOG.debug('Path %s matched pattern %s, ' + 'entry will be deleted: %s', + path, pattern.pattern, entry) + remove_boot_record(boot_num) + break + + def _run_efibootmgr(valid_efi_bootloaders, device, efi_partition, mount_point, label_suffix=None): """Executes efibootmgr and removes duplicate entries. diff --git a/ironic_python_agent/hardware.py b/ironic_python_agent/hardware.py index bc1b2161e..5c95b14a4 100644 --- a/ironic_python_agent/hardware.py +++ b/ironic_python_agent/hardware.py @@ -41,6 +41,7 @@ import yaml from ironic_python_agent import burnin from ironic_python_agent import disk_utils +from ironic_python_agent import efi_utils from ironic_python_agent import encoding from ironic_python_agent import errors from ironic_python_agent.extensions import base as ext_base @@ -86,6 +87,24 @@ RAID_APPLY_CONFIGURATION_ARGSINFO = { } } +DEFAULT_CLEAN_UEFI_NVRAM_MATCH_PATTERNS = [ + r'^HD\(', + r'shim.*\.efi', + r'grub.*\.efi' +] +DEPLOY_CLEAN_UEFI_NVRAM_ARGSINFO = { + "match_patterns": { + "description": ( + "Json blob contains a list of regex patterns where any UEFI " + "NVRAM entry matching that pattern will be deleted. " + "Default value is " + "'[\"{}\"]'".format('", "'.join( + DEFAULT_CLEAN_UEFI_NVRAM_MATCH_PATTERNS)) + ), + "required": False, + } +} + MULTIPATH_ENABLED = None @@ -2410,6 +2429,14 @@ class GenericHardwareManager(HardwareManager): 'reboot_requested': False, 'abortable': True }, + { + 'step': 'clean_uefi_nvram', + 'priority': 0, + 'interface': 'deploy', + 'reboot_requested': False, + 'abortable': True, + 'argsinfo': DEPLOY_CLEAN_UEFI_NVRAM_ARGSINFO, + }, { 'step': 'delete_configuration', 'priority': 0, @@ -2469,6 +2496,13 @@ class GenericHardwareManager(HardwareManager): 'reboot_requested': False, 'argsinfo': RAID_APPLY_CONFIGURATION_ARGSINFO, }, + { + 'step': 'clean_uefi_nvram', + 'priority': 0, + 'interface': 'deploy', + 'reboot_requested': False, + 'argsinfo': DEPLOY_CLEAN_UEFI_NVRAM_ARGSINFO, + }, { 'step': 'write_image', # NOTE(dtantsur): this step has to be proxied via an @@ -2558,6 +2592,32 @@ class GenericHardwareManager(HardwareManager): # TODO(TheJulia): Consider erase_devices and friends... return service_steps + def clean_uefi_nvram(self, node, ports, match_patterns=None): + """Clean UEFI NVRAM entries. + + :param node: A dictionary of the node object. + :param ports: A list of dictionaries containing information + of ports for the node. + :param match_patterns: A list of string regular expression patterns + where any matching entry will be deleted. + """ + if match_patterns is None: + match_patterns = DEFAULT_CLEAN_UEFI_NVRAM_MATCH_PATTERNS + validation_error = ('The match_patterns must be a list of strings: ' + '{}').format(match_patterns) + if not type(match_patterns) is list: + raise errors.InvalidCommandParamsError(validation_error) + patterns = [] + for item in match_patterns: + if not isinstance(item, str): + raise errors.InvalidCommandParamsError(validation_error) + try: + patterns.append(re.compile(item, flags=re.IGNORECASE)) + except re.error: + raise errors.InvalidCommandParamsError(validation_error) + + return efi_utils.clean_boot_records(patterns=patterns) + def apply_configuration(self, node, ports, raid_config, delete_existing=True): """Apply RAID configuration. diff --git a/ironic_python_agent/tests/unit/test_efi_utils.py b/ironic_python_agent/tests/unit/test_efi_utils.py index 935bee8ec..3132e26dd 100644 --- a/ironic_python_agent/tests/unit/test_efi_utils.py +++ b/ironic_python_agent/tests/unit/test_efi_utils.py @@ -11,6 +11,7 @@ # under the License. import os +import re import shutil import tempfile from unittest import mock @@ -203,10 +204,10 @@ class TestManageUefi(base.IronicAgentTest): self.assertEqual(5, mock_execute.call_count) mock_rescan.assert_called_once_with(self.fake_dev) - def test_get_boot_record(self, mock_utils_efi_part, - mock_get_part_path, - mock_get_part_uuid, mock_execute, - mock_rescan): + def test_get_boot_records(self, mock_utils_efi_part, + mock_get_part_path, + mock_get_part_uuid, mock_execute, + mock_rescan): efibootmgr_resp = """ BootCurrent: 0001 Timeout: 0 seconds @@ -296,6 +297,46 @@ Boot0009* Internal CD/DVD ROM Drive (UEFI) PciRoot(0x0)/Pci(0x11,0x0)/Sata( 'CDROM(1,0x265,0x2000)'), result[13]) + def test_clean_boot_records(self, mock_utils_efi_part, + mock_get_part_path, + mock_get_part_uuid, mock_execute, + mock_rescan): + efibootmgr_resp = """ +BootCurrent: 0001 +Timeout: 0 seconds +BootOrder: 0001,0000,001B,001C,001D,001E,001F,0020,0021,0022,0012,0011,0023,0024,0002 +Boot0000* Red Hat Enterprise Linux HD(1,GPT,34178504-2340-4fe0-8001-264372cf9b2d,0x800,0x64000)/File(\\EFI\\redhat\\grubx64.efi) +Boot0001* Fedora HD(1,GPT,da6b4491-61f2-42b0-8ab1-7c4a87317c4e,0x800,0x64000)/File(\\EFI\\fedora\\SHIMX64.EFI) +Boot0002* Linux-Firmware-Updater HD(1,GPT,da6b4491-61f2-42b0-8ab1-7c4a87317c4e,0x800,0x64000)/File(\\EFI\\fedora\\fwupdx64.efi) +Boot0003 ThinkShield secure wipe FvFile(3593a0d5-bd52-43a0-808e-cbff5ece2477) +Boot0004 LENOVO CLOUD VenMsg(bc7838d2-0f82-4d60-8316-c068ee79d25b,ad38ccbbf7edf04d959cf42aa74d3650)/Uri(https://download.lenovo.com/pccbbs/cdeploy/efi/boot.efi) +Boot0005 IDER BOOT CDROM PciRoot(0x0)/Pci(0x14,0x0)/USB(11,1) +Boot0006 ATA HDD VenMsg(bc7838d2-0f82-4d60-8316-c068ee79d25b,91af625956449f41a7b91f4f892ab0f6) +Boot0007* Hard drive C: VenHw(d6c0639f-c705-4eb9-aa4f-5802d8823de6)......................................................................................A.....................P.E.R.C. .H.7.3.0.P. .M.i.n.i.(.b.u.s. .1.8. .d.e.v. .0.0.)... +BootAAA8* IBA GE Slot 0100 v1588 BBS(128,IBA GE Slot 0100 v1588,0x0)........................B.............................................................A.....................I.B.A. .G.E. .S.l.o.t. .0.1.0.0. .v.1.5.8.8... +Boot0FF9* Virtual CD/DVD PciRoot(0x0)/Pci(0x14,0x0)/USB(13,0)/USB(3,0)/USB(1,0) +Boot123A* Integrated NIC 1 Port 1 Partition 1 VenHw(33391845-5f86-4e78-8fce-c4cff59f9bbb) +Boot000B* UEFI: PXE IPv4 Realtek PCIe 2.5GBE Family Controller PciRoot(0x0)/Pci(0x1c,0x0)/Pci(0x0,0x0)/MAC([REDACTED],0)/IPv4(0.0.0.00.0.0.0,0,0)..BO +Boot0008* Generic USB Boot UsbClass(ffff,ffff,255,255) +Boot0009* Internal CD/DVD ROM Drive (UEFI) PciRoot(0x0)/Pci(0x11,0x0)/Sata(1,65535,0)/CDROM(1,0x265,0x2000) +""".encode('utf-16') # noqa This is a giant literal string for testing. + mock_execute.return_value = (efibootmgr_resp, '') + patterns = [ + re.compile(r'^HD\(', flags=re.IGNORECASE), + re.compile(r'shim.*\.efi', flags=re.IGNORECASE), + re.compile(r'^UsbClass', flags=re.IGNORECASE), + ] + efi_utils.clean_boot_records(patterns) + + # Assert that entries 0000, 0001, 0002, and 0008 were deleted + mock_execute.assert_has_calls([ + mock.call('efibootmgr', '-v', binary=True), + mock.call('efibootmgr', '-b', '0000', '-B', binary=True), + mock.call('efibootmgr', '-b', '0001', '-B', binary=True), + mock.call('efibootmgr', '-b', '0002', '-B', binary=True), + mock.call('efibootmgr', '-b', '0008', '-B', binary=True) + ]) + @mock.patch.object(os.path, 'exists', lambda *_: False) @mock.patch.object(hardware, 'is_md_device', autospec=True) @mock.patch.object(efi_utils, '_get_efi_bootloaders', autospec=True) diff --git a/ironic_python_agent/tests/unit/test_hardware.py b/ironic_python_agent/tests/unit/test_hardware.py index 34f917b95..fca8c3773 100644 --- a/ironic_python_agent/tests/unit/test_hardware.py +++ b/ironic_python_agent/tests/unit/test_hardware.py @@ -15,6 +15,7 @@ import binascii import json import os +import re import shutil import stat import time @@ -29,6 +30,7 @@ import pyudev from stevedore import extension from ironic_python_agent import disk_utils +from ironic_python_agent import efi_utils from ironic_python_agent import errors from ironic_python_agent import hardware from ironic_python_agent import netutils @@ -162,6 +164,14 @@ class TestGenericHardwareManager(base.IronicAgentTest): 'reboot_requested': False, 'abortable': True }, + { + 'step': 'clean_uefi_nvram', + 'priority': 0, + 'interface': 'deploy', + 'reboot_requested': False, + 'abortable': True, + 'argsinfo': mock.ANY + }, { 'step': 'delete_configuration', 'priority': 0, @@ -2898,6 +2908,40 @@ class TestGenericHardwareManager(base.IronicAgentTest): self.assertEqual('2001:5678:5678:5678:5678:5678:5678:5678', self.hardware.get_bmc_v6address()) + @mock.patch.object(efi_utils, 'clean_boot_records', autospec=True) + def test_clean_uefi_nvram_defaults(self, mock_efi_utils): + self.hardware.clean_uefi_nvram(self.node, []) + mock_efi_utils.assert_called_once_with(patterns=[ + re.compile(r'^HD\(', flags=re.IGNORECASE), + re.compile(r'shim.*\.efi', flags=re.IGNORECASE), + re.compile(r'grub.*\.efi', flags=re.IGNORECASE) + ]) + + @mock.patch.object(efi_utils, 'clean_boot_records', autospec=True) + def test_clean_uefi_nvram(self, mock_efi_utils): + self.hardware.clean_uefi_nvram(self.node, [], match_patterns=[ + 'VenHw', 'VenMsg' + ]) + mock_efi_utils.assert_called_once_with(patterns=[ + re.compile(r'VenHw', flags=re.IGNORECASE), + re.compile(r'VenMsg', flags=re.IGNORECASE) + ]) + + @mock.patch.object(efi_utils, 'clean_boot_records', autospec=True) + def test_clean_uefi_invalid(self, mock_efi_utils): + # Not a list + self.assertRaises(errors.InvalidCommandParamsError, + self.hardware.clean_uefi_nvram, self.node, [], + match_patterns='VenHw') + # Not a list of strings + self.assertRaises(errors.InvalidCommandParamsError, + self.hardware.clean_uefi_nvram, self.node, [], + match_patterns=[True]) + # Not valid regular expression + self.assertRaises(errors.InvalidCommandParamsError, + self.hardware.clean_uefi_nvram, self.node, [], + match_patterns=[')oo(']) + @mock.patch.object(il_utils, 'execute', autospec=True) def test_validate_configuration_no_configuration(self, mocked_execute): self.assertRaises(errors.SoftwareRAIDError, diff --git a/releasenotes/notes/clean_uefi_nvram-554041f2e7b2d555.yaml b/releasenotes/notes/clean_uefi_nvram-554041f2e7b2d555.yaml new file mode 100644 index 000000000..5310332b8 --- /dev/null +++ b/releasenotes/notes/clean_uefi_nvram-554041f2e7b2d555.yaml @@ -0,0 +1,10 @@ +--- +features: + - | + Adds a deploy step ``clean_uefi_nvram`` to remove unrequired extra UEFI + NVRAM boot entries. By default any entry matching ``HD`` as the root device, + or with a ``shim`` or ``grub`` efi file in the path will be deleted, + ensuring that disk based boot entries are removed before the new entry is + created for the written image. The ``match_patterns`` parameter allows a + list of regular expressions to be passed, where a case insensitive search in + the device path will result in that entry being deleted. \ No newline at end of file