Message-ID: <ZBsIvLUBNwYAjNUK@FVFF77S0Q05N>
Date:   Wed, 22 Mar 2023 13:55:08 +0000
From:   Mark Rutland <mark.rutland@....com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     linux-kernel@...r.kernel.org, agordeev@...ux.ibm.com,
        aou@...s.berkeley.edu, bp@...en8.de, catalin.marinas@....com,
        dave.hansen@...ux.intel.com, davem@...emloft.net,
        gor@...ux.ibm.com, hca@...ux.ibm.com, linux-arch@...r.kernel.org,
        linux@...linux.org.uk, mingo@...hat.com, palmer@...belt.com,
        paul.walmsley@...ive.com, robin.murphy@....com, tglx@...utronix.de,
        viro@...iv.linux.org.uk, will@...nel.org
Subject: Re: [PATCH v2 1/4] lib: test copy_{to,from}_user()

On Tue, Mar 21, 2023 at 11:04:26AM -0700, Linus Torvalds wrote:
> On Tue, Mar 21, 2023 at 5:25 AM Mark Rutland <mark.rutland@....com> wrote:
> >
> > * arm64's copy_to_user() under-reports the number of bytes copied in
> >   some cases, e.g.
> 
> So I think this is the ok case.
> 
> > * arm's copy_to_user() under-reports the number of bytes copied in some
> >   cases, and both copy_to_user() and copy_from_user() don't guarantee
> >   that at least a single byte is copied when a partial copy is possible,
> 
> Again, this is ok historically.
> 
> > * i386's copy_from_user does not guarantee that at least a single byte
> >   is copied when a partial copit is possible, e.g.
> >
> >   | too few bytes consumed (offset=4093, size=8, ret=8)
> 
> And here's the real example of "we've always done this optimization".
> The exact details have differed, but the i386 case is the really
> really traditional one: it does word-at-a-time copies, and does *not*
> try to fall back to byte-wise copies. Never has.

Sure; I understand that. The reason for pointing this out is that Al was very
specific that implementations *must* guarantee this back in:

  https://lore.kernel.org/all/YNSyZaZtPTmTa5P8@zeniv-ca.linux.org.uk/

... and that this could be done by having the fixup handler try to copy a byte.

I had assumed that *something* depended upon that, but I don't know what that
something actually is.

I'm not wedded to the semantic either way; if that's not required I can drop it
from the tests.

> > * s390 passes all tests
> >
> > * sparc's copy_from_user() over-reports the number of bbytes copied in
> >   some caes, e.g.
> 
> So this case I think this is wrong, and an outright bug. That can
> cause people to think that uninitialized data is initialized, and leak
> sensitive information.

Agreed.

> > * x86_64 passes all tests
> 
> I suspect your testing is flawed due to being too limited, and x86-64
> having multiple different copying routines.

Sorry; I should've called that out explicitly. I'm aware I'm not testing all
the variants (I'd be happy to); I just wanted to check that I wasn't going off
into the weeds with the semantics first.

I probably should've sent this as an RFC...

> Yes, at some point we made everything be quite careful with
> "handle_tail" etc, but we end up still having things that fail early,
> and fail hard.
> 
> At a minimum, at least unsafe_copy_to_user() will fault and not do the
> "fill to the very last byte" case. Of course, that doesn't return a
> partial length (it only has a "fail" case), but it's an example of
> this whole thing where we haven't really been byte-exact when doing
> copies.

Sure; that does seem to be different structurally too, so it'd need to be
plumbed into the harness differently.

I'll note that's more like {get,put}_user() which similarly just have a fail
case (and a put_user() could do a parital write then fault).

> So again, I get the feeling that these rules may make sense from a
> validation standpoint, but I'm not 100% sure we should generally have
> to be this careful.

I'm more than happy to relax the tests (and the docs); I just need to know
where the boundary is between what we must guarantee and what's a nice-to-have.

Thanks,
Mark.

Powered by blists - more mailing lists