Re: [PATCH] x86/uaccess: Zero the 8-byte get_range case on failure

[David Gow]

On Wed, 31 Jul 2024 09:38:15 -0700 Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:

> On Wed, 31 Jul 2024 at 09:24, Linus Torvalds
> <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > My bad. My mental model these days is the 64-bit case, where the whole
> > 'check_range' thing is about address masking tricks, not the actual
> > conditional. So I didn't think of the "access_ok fails" case at all.
> 
> Actually, now that I said that out loud, it made me go "why aren't we
> doing that on 32-bit too?"
> 
> IOW, an alternative would be to just unify things more. Something like this?
> 
> *ENTIRELY UNTESTED*.

I tested this on everything I had on hand, and it passes the usercopy
KUnit tests on:
- QEMU, in every config I tried
- A Core i7-4770K (Haswell), under KVM on a 64-bit host kernel
- A Core 2 Duo E6600, bare metal
- A 486/DX2, bare metal

The 486 seemed to treat the wraparound a bit differently: it's triggering
a General Protection Fault, and so giving the WARN() normally reserved
for non-canonical addresses.

> 
> And no, this is not a NAK of David's patch. Last time I said "let's
> unify things", it caused this bug.
> 
> I'm claiming that finishing that unification will fix the bug again,
> and I *think* we leave that top address unmapped on 32-bit x86 too,
> but this whole trick does very much depend on that "access to all-one
> address is guaranteed to fail".
> 
> So this is the "maybe cleaner, but somebody really needs to
> double-check me" patch.
> 
>              Linus
>

I think this is right (there's definitely an unmapped page at the top),
but I'd want someone who knows better to verify that this won't do
something weird with segmentation and/or speculation with the 8-byte case
(if vm.mmap_min_addr is 0 and someone's mapped page 0).

The Intel manuals are very cagey about what happens with wraparound and
segmentation, and definitely seem to suggest that it's implementation
dependent:

"When the effective limit is FFFFFFFFH (4 GBytes), these accesses may or may not
cause the indicated exceptions.  Behavior is implementation-specific and may
vary from one execution to another."

So I'm a little worried that there might be more cases where this works
differently. Does anyone know for sure if it's worth risking it?

Regardless, I tried making the same changes to put_user, and those work
equally well and pass the same tests (including with the WARN() on 486).
Combined patch below.

Cheers,
-- David

---