rework ATA secure erase

hdparm versions prior to 9.51 interpret the value, NULL, as a
password with string value: "NULL".

Example output of hdparm with NULL password:

    [root@localhost ~]# hdparm --user-master u --security-unlock NULL /dev/sda
    security_password="NULL"

    /dev/sda:
    Issuing SECURITY_UNLOCK command, password="NULL", user=user
    SECURITY_UNLOCK: Input/output error

Example output of hdparm with "" as password:

    [root@localhost ~]# hdparm --user-master u --security-unlock "" /dev/sda
    security_password=""

    /dev/sda:
     Issuing SECURITY_UNLOCK command, password="", user=user

Note the values of security_password in the output above. The output
was observed on a CentOS 7 system, which ships hdparm 9.43 in the
offical repositories.

This change attempts to unlock the drive with the empty string if an
unlock with NULL was unsucessful.

Issuing a security-unlock will cause a state transition from SEC4
(security enabled, locked, not frozen) to SEC5 (security enabled,
unlocked, not frozen). In order to check that a password unlock attempt
was successful it makes sense to check that the drive is in the unlocked
state (a necessary condition for SEC5). Only after all unlock attempts
fail, do we consider the drive out of our control.

The conditions to check the drive is in the right state have been
adjusted to ensure that the drive is in the SEC5 state prior to issuing
a secure erase. Previously, on the "recovery from previous fail" path,
the security state was asserted to be "not enabled" after an unlock -
this could never have been the case.

A good overview of the ATA security states can be found here:

  http://www.admin-magazine.com/Archive/2014/19/Using-the-ATA-security-features-of-modern-hard-disks-and-SSDs

Change-Id: Ic24b706a04ff6c08d750b9e3d79eb79eab2952ad
Story: 2001762
Task: 12161
Story: 2001763
Task: 12162
This commit is contained in:
Will Szumski 2018-05-10 21:53:44 +01:00 committed by Julia Kreger
parent a69ccee287
commit aaf76e2cfb
2 changed files with 106 additions and 48 deletions

View File

@ -898,39 +898,57 @@ class GenericHardwareManager(HardwareManager):
if 'supported' not in security_lines:
return False
if 'enabled' in security_lines:
# Attempt to unlock the drive in the event it has already been
# locked by a previous failed attempt.
try:
utils.execute('hdparm', '--user-master', 'u',
'--security-unlock', 'NULL', block_device.name)
security_lines = self._get_ata_security_lines(block_device)
except processutils.ProcessExecutionError as e:
raise errors.BlockDeviceEraseError('Security password set '
'failed for device '
'%(name)s: %(err)s' %
{'name': block_device.name,
'err': e})
if 'enabled' in security_lines:
raise errors.BlockDeviceEraseError(
('Block device {} already has a security password set'
).format(block_device.name))
# At this point, we could be SEC1,2,4,5,6
if 'not frozen' not in security_lines:
# In SEC2 or 6
raise errors.BlockDeviceEraseError(
('Block device {} is frozen and cannot be erased'
).format(block_device.name))
try:
utils.execute('hdparm', '--user-master', 'u',
'--security-set-pass', 'NULL', block_device.name)
except processutils.ProcessExecutionError as e:
raise errors.BlockDeviceEraseError('Security password set '
'failed for device '
'%(name)s: %(err)s' %
{'name': block_device.name,
'err': e})
# At this point, we could be in SEC1,4,5
# Attempt to unlock the drive in the event it has already been
# locked by a previous failed attempt. We try the empty string as
# versions of hdparm < 9.51, interpreted NULL as the literal string,
# "NULL", as opposed to the empty string.
unlock_passwords = ['NULL', '']
for password in unlock_passwords:
if 'not locked' in security_lines:
break
try:
utils.execute('hdparm', '--user-master', 'u',
'--security-unlock', password,
block_device.name)
except processutils.ProcessExecutionError as e:
LOG.info('Security unlock failed for device '
'%(name)s using password "%(password)s": %(err)s',
{'name': block_device.name,
'password': password,
'err': e})
security_lines = self._get_ata_security_lines(block_device)
# If the unlock failed we will still be in SEC4, otherwise, we will be
# in SEC1 or SEC5
if 'not locked' not in security_lines:
# In SEC4
raise errors.BlockDeviceEraseError(
('Block device {} already has a security password set'
).format(block_device.name))
# At this point, we could be in SEC1 or 5
if 'not enabled' in security_lines:
# SEC1. Try to transition to SEC5 by setting empty user
# password.
try:
utils.execute('hdparm', '--user-master', 'u',
'--security-set-pass', 'NULL', block_device.name)
except processutils.ProcessExecutionError as e:
error_msg = ('Security password set failed for device '
'{name}: {err}'
).format(name=block_device.name, err=e)
raise errors.BlockDeviceEraseError(error_msg)
# Use the 'enhanced' security erase option if it's supported.
erase_option = '--security-erase'
@ -949,10 +967,12 @@ class GenericHardwareManager(HardwareManager):
# Verify that security is now 'not enabled'
security_lines = self._get_ata_security_lines(block_device)
if 'not enabled' not in security_lines:
# Not SEC1 - fail
raise errors.BlockDeviceEraseError(
('An unknown error occurred erasing block device {}'
).format(block_device.name))
# In SEC1 security state
return True
def get_bmc_address(self):

View File

@ -113,7 +113,7 @@ HDPARM_INFO_TEMPLATE = (
'\tMaster password revision code = 65534\n'
'\t%(supported)s\n'
'\t%(enabled)s\n'
'\tnot\tlocked\n'
'\t%(locked)s\n'
'\t%(frozen)s\n'
'\tnot\texpired: security count\n'
'\t%(enhanced_erase)s\n'
@ -1441,15 +1441,49 @@ class TestGenericHardwareManager(base.IronicAgentTest):
'shred', '--force', '--zero', '--verbose', '--iterations', '1',
'/dev/sda')
@mock.patch.object(utils, 'execute', autospec=True)
def test_erase_block_device_ata_security_unlock_fallback_pass(
self, mocked_execute):
hdparm_output = create_hdparm_info(
supported=True, enabled=True, locked=True
)
hdparm_output_unlocked = create_hdparm_info(
supported=True, enabled=True, frozen=False, enhanced_erase=False)
hdparm_output_not_enabled = create_hdparm_info(
supported=True, enabled=False, frozen=False, enhanced_erase=False)
mocked_execute.side_effect = [
(hdparm_output, ''),
processutils.ProcessExecutionError(), # NULL fails to unlock
(hdparm_output, ''), # recheck security lines
None, # security unlock with ""
(hdparm_output_unlocked, ''),
'',
(hdparm_output_not_enabled, '')
]
block_device = hardware.BlockDevice('/dev/sda', 'big', 1073741824,
True)
self.hardware.erase_block_device(self.node, block_device)
mocked_execute.assert_any_call('hdparm', '--user-master', 'u',
'--security-unlock', '', '/dev/sda')
@mock.patch.object(hardware.GenericHardwareManager, '_shred_block_device',
autospec=True)
@mock.patch.object(utils, 'execute', autospec=True)
def test_erase_block_device_ata_security_enabled(
self, mocked_execute, mock_shred):
# Tests that an exception is thrown if all of the recovery passwords
# fail to unlock the device without throwing exception
hdparm_output = create_hdparm_info(
supported=True, enabled=True, frozen=False, enhanced_erase=False)
supported=True, enabled=True, locked=True)
mocked_execute.side_effect = [
(hdparm_output, ''),
None,
(hdparm_output, ''),
None,
(hdparm_output, ''),
None,
(hdparm_output, '')
@ -1457,12 +1491,15 @@ class TestGenericHardwareManager(base.IronicAgentTest):
block_device = hardware.BlockDevice('/dev/sda', 'big', 1073741824,
True)
self.assertRaises(
errors.IncompatibleHardwareMethodError,
self.hardware.erase_block_device,
self.node,
block_device)
mocked_execute.assert_any_call('hdparm', '--user-master', 'u',
'--security-unlock', '', '/dev/sda')
mocked_execute.assert_any_call('hdparm', '--user-master', 'u',
'--security-unlock', 'NULL', '/dev/sda')
self.assertFalse(mock_shred.called)
@mock.patch.object(hardware.GenericHardwareManager, '_shred_block_device',
@ -1471,7 +1508,7 @@ class TestGenericHardwareManager(base.IronicAgentTest):
def test_erase_block_device_ata_security_enabled_unlock_attempt(
self, mocked_execute, mock_shred):
hdparm_output = create_hdparm_info(
supported=True, enabled=True, frozen=False, enhanced_erase=False)
supported=True, enabled=True, locked=True)
hdparm_output_not_enabled = create_hdparm_info(
supported=True, enabled=False, frozen=False, enhanced_erase=False)
@ -1493,34 +1530,36 @@ class TestGenericHardwareManager(base.IronicAgentTest):
@mock.patch.object(utils, 'execute', autospec=True)
def test__ata_erase_security_enabled_unlock_exception(
self, mocked_execute):
# test that an exception is thrown when security unlock fails with
# ProcessExecutionError
hdparm_output = create_hdparm_info(
supported=True, enabled=True, frozen=False, enhanced_erase=False)
supported=True, enabled=True, locked=True)
mocked_execute.side_effect = [
(hdparm_output, ''),
processutils.ProcessExecutionError()
processutils.ProcessExecutionError(),
(hdparm_output, ''),
processutils.ProcessExecutionError(),
(hdparm_output, ''),
]
block_device = hardware.BlockDevice('/dev/sda', 'big', 1073741824,
True)
self.assertRaises(errors.BlockDeviceEraseError,
self.hardware._ata_erase,
block_device)
mocked_execute.assert_any_call('hdparm', '--user-master', 'u',
'--security-unlock', '', '/dev/sda')
mocked_execute.assert_any_call('hdparm', '--user-master', 'u',
'--security-unlock', 'NULL', '/dev/sda')
@mock.patch.object(utils, 'execute', autospec=True)
def test__ata_erase_security_enabled_set_password_exception(
self, mocked_execute):
hdparm_output = create_hdparm_info(
supported=True, enabled=True, frozen=False, enhanced_erase=False)
hdparm_output_not_enabled = create_hdparm_info(
supported=True, enabled=False, frozen=False, enhanced_erase=False)
mocked_execute.side_effect = [
(hdparm_output, ''),
'',
(hdparm_output_not_enabled, ''),
'',
processutils.ProcessExecutionError()
]
@ -1534,17 +1573,14 @@ class TestGenericHardwareManager(base.IronicAgentTest):
@mock.patch.object(utils, 'execute', autospec=True)
def test__ata_erase_security_erase_exec_exception(
self, mocked_execute):
# Exception on security erase
hdparm_output = create_hdparm_info(
supported=True, enabled=True, frozen=False, enhanced_erase=False)
hdparm_output_not_enabled = create_hdparm_info(
supported=True, enabled=False, frozen=False, enhanced_erase=False)
mocked_execute.side_effect = [
(hdparm_output, '', '-1'),
'',
(hdparm_output_not_enabled, ''),
'',
processutils.ProcessExecutionError()
'', # security-set-pass
processutils.ProcessExecutionError() # security-erase
]
block_device = hardware.BlockDevice('/dev/sda', 'big', 1073741824,
@ -2003,8 +2039,8 @@ class TestModuleFunctions(base.IronicAgentTest):
mock.call('iscsistart', '-f')])
def create_hdparm_info(supported=False, enabled=False, frozen=False,
enhanced_erase=False):
def create_hdparm_info(supported=False, enabled=False, locked=False,
frozen=False, enhanced_erase=False):
def update_values(values, state, key):
if not state:
@ -2013,12 +2049,14 @@ def create_hdparm_info(supported=False, enabled=False, frozen=False,
values = {
'supported': '\tsupported',
'enabled': '\tenabled',
'locked': '\tlocked',
'frozen': '\tfrozen',
'enhanced_erase': '\tsupported: enhanced erase',
}
update_values(values, supported, 'supported')
update_values(values, enabled, 'enabled')
update_values(values, locked, 'locked')
update_values(values, frozen, 'frozen')
update_values(values, enhanced_erase, 'enhanced_erase')