Hi all,
while staring at the block layer code I found what I think is a major
security issue with the use of REQ_OP_SECURE_ERASE.
The issue is not about the actual protocol implementation, which only
exists for eMMC [1], but about we handle issuing the operation in the
block layer. That is done through __blkdev_issue_discard, which
takes various parameters into account to align the issue discard
request to what the hardware prefers. Which is perfectly fine for
discard as an advisory operation, but deadly for an operation that
wants to make data inaccessible. The problem has existed ever since
secure erase support was added to the kernel with commit
8d57a98ccd0b ("block: add secure discard"), which added secure erase
support as a REQ_SECURE flag to the discard operation.
The ioctl added there also as the only users for a long time, until f2fs
added a second (really strange) user that uses secure erase if offered by
the device but otherwise plain old discard: 9af846486d78
("f2fs: add F2FS_IOC_SEC_TRIM_FILE ioctl") which seems to treat the
secure discard as nice to have but actually is fine with data leaks
from the use of discard or an incorrect implementation of secure erase.
My preference would be to just remove this ill designed feature entirely.
The alternative 1 in this thead does just that. Alternative 2 tries to
fix it instead, but I haven't bee nable to get any interested party to
actually test in more than three eeks, suggesting we're better off
removing the code.
[1] which is rather dubious as well, as sector based secure erase in
flash based media can't really work due to the lack of in-place write
support. At best it is the equivalent for a Write Same or Write Zeroes
command without deterministic data on the next read.
On Wed, Mar 16, 2022 at 10:37:40AM +0100, Christoph Hellwig wrote:
> Hi all,
>
> while staring at the block layer code I found what I think is a major
> security issue with the use of REQ_OP_SECURE_ERASE.
>
> The issue is not about the actual protocol implementation, which only
> exists for eMMC [1], but about we handle issuing the operation in the
> block layer. That is done through __blkdev_issue_discard, which
> takes various parameters into account to align the issue discard
> request to what the hardware prefers. Which is perfectly fine for
> discard as an advisory operation, but deadly for an operation that
> wants to make data inaccessible. The problem has existed ever since
> secure erase support was added to the kernel with commit
> 8d57a98ccd0b ("block: add secure discard"), which added secure erase
> support as a REQ_SECURE flag to the discard operation.
__blkdev_issue_discard() can break up the region into multiple bios, but I don't
see where it actually skips parts of the region. Can you explain more
specifically where the problem is?
- Eric
On Wed, Mar 16, 2022 at 06:05:29PM +0000, Eric Biggers wrote:
> On Wed, Mar 16, 2022 at 10:37:40AM +0100, Christoph Hellwig wrote:
> > Hi all,
> >
> > while staring at the block layer code I found what I think is a major
> > security issue with the use of REQ_OP_SECURE_ERASE.
> >
> > The issue is not about the actual protocol implementation, which only
> > exists for eMMC [1], but about we handle issuing the operation in the
> > block layer. That is done through __blkdev_issue_discard, which
> > takes various parameters into account to align the issue discard
> > request to what the hardware prefers. Which is perfectly fine for
> > discard as an advisory operation, but deadly for an operation that
> > wants to make data inaccessible. The problem has existed ever since
> > secure erase support was added to the kernel with commit
> > 8d57a98ccd0b ("block: add secure discard"), which added secure erase
> > support as a REQ_SECURE flag to the discard operation.
>
> __blkdev_issue_discard() can break up the region into multiple bios, but I don't
> see where it actually skips parts of the region. Can you explain more
> specifically where the problem is?
>
> - Eric
I'm also not seeing it.
As I read the __blkdev_issue_discard() function it uses
discard_granularity to define the required sectors (req_sects) for each
bio. req_sects can change on every iteration of the while loop, but
all consecutive bios then start where the previous one ended.
Am I missing something?
Joel
On Wed, Mar 16, 2022 at 06:05:29PM +0000, Eric Biggers wrote:
> __blkdev_issue_discard() can break up the region into multiple bios, but I don't
> see where it actually skips parts of the region. Can you explain more
> specifically where the problem is?
You're right. We used to skip misaligned parts, but that was actually
fixed two years ago.