Code review comment for ~gpiccoli/maas:nvme_secure_erase

Revision history for this message
Guilherme G. Piccoli (gpiccoli) wrote :

Hi Igor, thanks again for you feedback. I think we are talking about 2 different features!

(a) Supporting NVMe secure erase. Currently it is not *supported*. So, users with NVMe drives cannot at all have them securely erased. This is what we are addressing here, AFAIK.

(b) To fail the erase operation if a secure erase is not performed well (or not supported). I think for this item, the best would be to have a new checkbox in MAAS user interface, something like "Fail if not secure erased".

Right now, the way secure erase is implemented is an attempt to do the secure erase operation and if it does not complete properly or if it's not supported, we fallback to zeroing the device. I guess we can discuss the change to a more strict mode with MAAS team, but I consider it a different feature, which is not in the scope for this proposal.

Finally, regarding the validation if secure erase was correct, like by writing data in the device and checking back after the secure operation: in a first moment that seems a good idea. But..how can we be sure?

Imagine we have a 1TB NVMe device , and due to the way FW works, it returns secure erase as completed but it took some more time and FW continue working in background. What if we write 1G of data, secure erase the drive and read back 1G, all "wrong" data (meaning secure erase worked), but...after the 500G offset in the disk, the data is not secure erased yet? Then if an user hard power-off the system (like removing the power cable), it is still prone to a cold-boot attack.
I know I'm a bit "philosophical" here, and that hypothesis I suggested above is almost nonsense, but by not trusting the tool, we should (for consistency) not trust the firmware, so...what to do?

I vote to trust the tool/firmware in a first moment, then we could improve that (although I'm not sure how to be fully guarded that secure erase worked). Perhaps nvme sanitize for devices that support that?

Cheers,

Guilherme

« Back to merge proposal