Re: [PATCH] btrfs: replace a wrong memset() with memzero_page()

[Filipe Manana <fdmanana@kernel.org>]

On Wed, Mar 30, 2022 at 12:03:36PM +0100, Graham Cobb wrote:
> 
> On 30/03/2022 11:42, Qu Wenruo wrote:
> > 
> > 
> > On 2022/3/30 18:34, Filipe Manana wrote:
> >> On Wed, Mar 30, 2022 at 10:27:53AM +0100, Filipe Manana wrote:
> >>> On Wed, Mar 30, 2022 at 07:52:04AM +0800, Qu Wenruo wrote:
> >>>>
> >>>>
> >>>> On 2022/3/29 19:39, Filipe Manana wrote:
> >>>>> On Tue, Mar 29, 2022 at 06:49:10PM +0800, Qu Wenruo wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 2022/3/29 17:57, Filipe Manana wrote:
> >>>>>>> On Mon, Mar 28, 2022 at 08:51:21PM +0200, David Sterba wrote:
> >>>>>>>> On Fri, Mar 25, 2022 at 05:37:59PM +0800, Qu Wenruo wrote:
> >>>>>>>>> The original code is not really setting the memory to 0x00 but
> >>>>>>>>> 0x01.
> >>>>>>>>>
> >>>>>>>>> To prevent such problem from happening, use memzero_page()
> >>>>>>>>> instead.
> >>>>>>>>
> >>>>>>>> This should at least mention we think that setting it to 0 is
> >>>>>>>> right, as
> >>>>>>>> you call it wrong but give no hint why it's thought to be wrong.
> >>>>>>>
> >>>>>>> My guess is that something different from zero makes it easier to
> >>>>>>> spot
> >>>>>>> the problem in user space, as 0 is not uncommon (holes,
> >>>>>>> prealloced extents)
> >>>>>>> and may get unnoticed by applications/users.
> >>>>>>
> >>>>>> OK, that makes some sense.
> >>>>>>
> >>>>>> But shouldn't user space tool get an -EIO directly?
> >>>>>
> >>>>> It should.
> >>>>>
> >>>>> But even if applications get -EIO, they may often ignore return
> >>>>> values.
> >>>>> It's their fault, but if we can make it less likely that errors are
> >>>>> not noticed,
> >>>>> the better. I think we all did often, ignore all or just some
> >>>>> return values
> >>>>> from read(), write(), open(), etc.
> >>>>>
> >>>>> One recent example is the MariaDB case with io-uring. They were
> >>>>> reporting
> >>>>> corruption to the users, but the problem is that didn't properly check
> >>>>> return values, ignoring partial reads and treating them as success:
> >>>>>
> >>>>> https://jira.mariadb.org/browse/MDEV-27900?focusedCommentId=216582&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-216582
> >>>>>
> >>>>>
> >>>>> The data was fine, not corrupted, they just didn't deal with
> >>>>> partial reads
> >>>>> and then read the remaining data when a read returns less data than
> >>>>> expected.
> >>>>
> >>>> Then can we slightly improve the filling pattern?
> >>>>
> >>>> Instead of 0x01, introduce some poison value?
> >>>
> >>> Why isn't 0x01 a good enough value?
> >>>
> >>> A long range of 0x01 values is certainly unexpected in text files,
> >>> and very likely
> >>> on binary formats as well. Or do you think there's some case where
> >>> long sequences
> >>> of 0x01 are common and not unexpected?
> >>>
> >>>>
> >>>> And of course, change the lable "zeroit" to something like "poise_it"?
> >>>
> >>> "poison_it", poise has a very different and unrelated meaning in
> >>> English.
> >>
> >> It's also worth considering if we should change the page content at all.
> >>
> >> Adding a poison value makes it easier to detect the corruption, both for
> >> developers and for sloppy applications that don't check error values.
> >>
> >> However people often want to still have access to the corrupted data,
> >> they
> >> can tolerate a few corrupted bytes and find the remaining useful. This
> >> has
> >> been requested a few times in the past.
> > 
> > This looks more favorable.
> > 
> > And I didn't think the change would break anything.
> > 
> > For proper user space programs checking the error, they know it's an
> > -EIO and will error out.
> > 
> > For bad programs not checking the error, it will just read the corrupted
> > data, and may even pass its internal sanity checks (if the corrupted
> > bytes are not in some critical part).
> > 
> > Or is there something we haven't taken into consideration?
> 
> Well.... If an error occurred something has gone wrong. It could be a
> simple bit flip in the storage itself, or it could be something else.
> The something else could be a software or firmware bug, or a memory
> corruption or memory controller power fluctuation blip, or .... I guess
> it might even be possible in some architectures that the problem could
> result in page table updates validating a page that actually was never
> written to at all and could still contain some previous contents.
> 
> In a time when we worry about things like Spectre, Meltdown, Rowhammer,
> stale cache leakage, etc it may be a good security principle that data
> left over from failed operations should never be made visible to user mode.
> 
> Possibly unnecessary worrying, but then who thought that jump prediction
> would create a side-channel for exposing data that can actually be
> exploited in real life.

Hum, then a filesystem that doesn't have data checksums (the vast majority of
them), like ext4, xfs, etc, has a serious security flaw, no? You ask to read an
extent, and it returns whatever is on disk, even if it has suffered some corruption.

Shall we open a CVE, and force all filesystems to implement data checksums? ;)
We would also have to drop support for nodatacow and nodatasum from btrfs.


> 
> Graham