LinuxLists.cc - [GIT PULL] x86/mm for 6.4

[permalink] [raw]

Subject: Re: [GIT PULL] x86/mm for 6.4

On Thu, Apr 27, 2023 at 3:57 PM Dave Hansen <[email protected]> wrote:
>
> Please pull some x86/mm changes for 6.4. The only content here is
> solely a new revision of Kirill's Linear Address Masking implementation.

So I was waiting for this for my final piece of the x86 user copy
changes, so here goes...

I think we should now make 'access_ok()' do the same thing that
get/put_user() do with the LAM code: only worry about the sign bit.

So here's my suggested change on top of the current tree. Comments?

PeterZ also added to the cc, because he's the source of that
WARN_ON_IN_IRQ() in the x86 'access_ok()' macro. That's the only
reason x86 has its own copy of that.

I wonder if that WARN_ON_IN_IRQ() should just be removed, or perhaps
moved into the generic code in <asm-generic/access_ok.h>?

Linus

Attachments:

0001-x86-64-make-access_ok-independent-of-LAM.patch (7.98 kB)

2023-04-28 20:21:40

[permalink] [raw]

Subject: Re: [GIT PULL] x86/mm for 6.4

On Fri, Apr 28, 2023 at 1:07 PM Linus Torvalds
<[email protected]> wrote:
>
> So here's my suggested change on top of the current tree. Comments?

Oh, and I wanted to particularly mention that

We could probably just do that "check only starting address" for any
arbitrary range size: realistically all kernel accesses to user space
will be done starting at the low address. But let's leave that kind of
optimization for later. As it is, this already allows us to generate
simpler code and not worry about any tag bits in the address.

part of the commit log.

Right now that patch only simplifies the range check for when the
compiler statically knows that the range is small (which does happen,
but not very often, because 'copy_to/from_user()' isn't inlined on
x86-64, so the compiler doesn't actually see the constant size case
that is very common there).

However, that "check the end of the range" is sometimes actually
fairly complicated code, and it would be nice to drop that entirely.

See for example the fs/readdir.c case, where the length of the access
is kind of annoying:

if (!user_write_access_begin(dirent,
(unsigned long)(dirent->d_name + namlen + 1) -
(unsigned long)dirent))
goto efault;

and there really isn't any actual reason to check the end of the
access on x86: if the beginning address has the low bit clear, it
doesn't really matter what the end is, because we'll either have a
good area, or we'll fault in the non-canonical area even if the sign
changes.

So being careful about the range is kind of annoying, when we don't
really need it.

Linus

2023-04-29 00:50:06

[permalink] [raw]

Subject: Re: [GIT PULL] x86/mm for 6.4

On Fri, Apr 28, 2023 at 01:15:33PM -0700, Linus Torvalds wrote:
> On Fri, Apr 28, 2023 at 1:07 PM Linus Torvalds
> <[email protected]> wrote:
> >
> > So here's my suggested change on top of the current tree. Comments?
>
> Oh, and I wanted to particularly mention that
>
> We could probably just do that "check only starting address" for any
> arbitrary range size: realistically all kernel accesses to user space
> will be done starting at the low address. But let's leave that kind of
> optimization for later. As it is, this already allows us to generate
> simpler code and not worry about any tag bits in the address.
>
> part of the commit log.
>
> Right now that patch only simplifies the range check for when the
> compiler statically knows that the range is small (which does happen,
> but not very often, because 'copy_to/from_user()' isn't inlined on
> x86-64, so the compiler doesn't actually see the constant size case
> that is very common there).

BTW, I think the static check can be relaxed. Checking size against
PAGE_SIZE is rather conservative: there's 8 TB (or 4 PB for 5-level
paging) guard hole at the begging of kernel address space.

> However, that "check the end of the range" is sometimes actually
> fairly complicated code, and it would be nice to drop that entirely.
>
> See for example the fs/readdir.c case, where the length of the access
> is kind of annoying:
>
> if (!user_write_access_begin(dirent,
> (unsigned long)(dirent->d_name + namlen + 1) -
> (unsigned long)dirent))
> goto efault;
>
> and there really isn't any actual reason to check the end of the
> access on x86: if the beginning address has the low bit clear, it
> doesn't really matter what the end is, because we'll either have a
> good area, or we'll fault in the non-canonical area even if the sign
> changes.
>
> So being careful about the range is kind of annoying, when we don't
> really need it.

Hm. Is there anybody who access high to low after the check (glibc
memcpy() bug flashbacks)? Or not in any particular order?

--
Kiryl Shutsemau / Kirill A. Shutemov

2023-04-29 01:07:09

[permalink] [raw]

Subject: Re: [GIT PULL] x86/mm for 6.4

On Fri, Apr 28, 2023 at 5:38 PM Kirill A. Shutemov <[email protected]> wrote:
>
> BTW, I think the static check can be relaxed. Checking size against
> PAGE_SIZE is rather conservative: there's 8 TB (or 4 PB for 5-level
> paging) guard hole at the begging of kernel address space.

So I don't worry about the size per se - we just don't have any
constant sized accesses that are bigger than a page.

The constant-sized case is for things like structures being copied to
user space.

And having a bug gap is nice for the suzkaller case, although I don't
think that GP fault has triggered lately (or ever, I don't remember).
Having random system call arguments that trigger "oh, this is in the
non-canonical region" is a good thing.

> > So being careful about the range is kind of annoying, when we don't
> > really need it.
>
> Hm. Is there anybody who access high to low after the check (glibc
> memcpy() bug flashbacks)? Or not in any particular order?

Yeah, I can't think of a single case, which is why it seems so silly
to even bother.

Almost all real life cases end up being limited by things like the
page/folio size.

We do have exceptions, like module loading etc that might copy a
bigger area from user space, but no, we don't have any backwards
copies.

So you'd almost have to have some "access_ok()" followed by random
access with a user-controlled offset, and that seems nonsensical and
fundamentally impossible anyway.

But just because I can't think of it, and go "that would be insane"
doesn't mean that some driver ioctl interface might not try it.

Which is why I think having others look at it would be a good idea.

Linus

2023-05-02 15:50:32

[permalink] [raw]

Subject: Re: [GIT PULL] x86/mm for 6.4

On 4/28/23 17:38, Kirill A. Shutemov wrote:
> BTW, I think the static check can be relaxed. Checking size against
> PAGE_SIZE is rather conservative: there's 8 TB (or 4 PB for 5-level
> paging) guard hole at the begging of kernel address space.

Whatever we relax it to, let's make sure we get a note in
Documentation/x86/x86_64/mm.rst. But that's totally minor and we can
fix it up later.

Have anyone seen any actual code generation difference between:

return (long)ptr >= 0;

and

return !((unsigned long)ptr & (1UL<<(BITS_PER_LONG-1)));

? I'm seeing gcc generate the same code for both the <=PAGE_SIZE side
and the 'sum' side.

It's longer, but I'd rather read the explicit "check bit 63" than the
positive/negative address space thing. I certainly grok both, but have
to think through the "(long)ptr >= 0" check every time.

I guess it also wouldn't matter as much either if we hid it in a helper
like the attached patch and I didn't have to read it twice. ;)

Attachments:

ptr_in_user_half.patch (2.48 kB)

2023-05-02 16:16:03

[permalink] [raw]

Subject: Re: [GIT PULL] x86/mm for 6.4

On Tue, May 2, 2023 at 8:42 AM Dave Hansen <[email protected]> wrote:
>
> Have anyone seen any actual code generation difference between:
>
> return (long)ptr >= 0;
>
> and
>
> return !((unsigned long)ptr & (1UL<<(BITS_PER_LONG-1)));

No, as far as I know, they both generate the same code.

> It's longer, but I'd rather read the explicit "check bit 63" than the
> positive/negative address space thing. I certainly grok both, but have
> to think through the "(long)ptr >= 0" check every time.

I'm very much the other way. I think it's much clearer to say "check
the sign bit".

Doing the explicit bit check means that I have to look at what the bit
number is, and that is a much more complex expression.

In fact, I'd find it easier to read

return !((unsigned long)ptr & (1UL<< 63));

just because then you go "Oh, checking bit 63" without having to parse
the expression.

But even then I find the '!' is easy to miss, so you really have to parse it.

But:

> I guess it also wouldn't matter as much either if we hid it in a helper
> like the attached patch and I didn't have to read it twice. ;)

Yeah, I think that's a good solution.

Linus

2023-05-02 20:21:24

[permalink] [raw]

Subject: Re: [GIT PULL] x86/mm for 6.4

On Tue, May 2, 2023 at 9:00 AM Linus Torvalds
<[email protected]> wrote:
>
> > I guess it also wouldn't matter as much either if we hid it in a helper
> > like the attached patch and I didn't have to read it twice. ;)
>
> Yeah, I think that's a good solution.

Hmm. And as I was rebasing the patch to fix up my patch, I realized
that the current -git top-of-tree state is actually broken.

That

#define access_ok(addr, size) \
({ \
WARN_ON_IN_IRQ(); \
likely(__access_ok(untagged_addr(addr), size)); \
})

is actually *wrong* in two ways.

Now, in my original patch, I added a comment about how that
"WARN_ON_IN_IRQ()" is bogus and this shouldn't be x86-specific at all.

I ended up going back in time to see why it was added, and I think it
was added because we used to access 'current' in access_ok(), due to
it using that user_addr_max() thing:

likely(!__range_not_ok(addr, size, user_addr_max()));

but that was all removed by the set_fs() removal by Christoph Hellwig.

So there is actually nothing task-related in "access_ok()" any more,
and any warning about using it in the wrong context is just bogus.
That warning simply shouldn't exist any more (or maybe it should be in
a different place, like the actual user copy functions)

But that's actually not the problem with the above.

No, the problem is that probably *because* "access_ok()" has that
warning, not all users use "access_ok()" at all. We have places that
use "__access_ok()" instead. Like copy_from_nmi().

So now copy_from_nmi() doesn't do the untagging, so if you were to use
tagged pointers for the stack, you'd not get stack traces.

End result: I think that

(a) that WARN_ON_IN_IRQ() is actively detrimental and causes problems

(b) the current "use untagged_addr() in access_ok()" model is also broken

and my patch - which was meant to just improve code generation -
actually fixes this.

Linus

2023-05-03 00:56:39

[permalink] [raw]

Subject: Re: [GIT PULL] x86/mm for 6.4

On 5/2/23 13:14, Linus Torvalds wrote:
> No, the problem is that probably *because* "access_ok()" has that
> warning, not all users use "access_ok()" at all. We have places that
> use "__access_ok()" instead. Like copy_from_nmi().
>
> So now copy_from_nmi() doesn't do the untagging, so if you were to use
> tagged pointers for the stack, you'd not get stack traces.
>
> End result: I think that
>
> (a) that WARN_ON_IN_IRQ() is actively detrimental and causes problems
>
> (b) the current "use untagged_addr() in access_ok()" model is also broken

Ugh, yes.

The fallout seems limited to (probably) perf and tracing poking at user
stack frames. But, yes, it definitely looks broken there.

While I bet we could shoehorn the existing tlbstate checks into the
__access_ok() sites, I also vastly prefer the high bit checks in
access_ok() instead. The less state we have to consult, the better.

Once the WARN_ON_IN_IRQ() is gone, it seems like it's just a matter of
collapsing __access_ok() into access_ok() and converting the (~3) callers.

2023-05-03 01:20:26

[permalink] [raw]

Subject: Re: [GIT PULL] x86/mm for 6.4

On Tue, May 2, 2023 at 5:53 PM Dave Hansen <[email protected]> wrote:
>
> The fallout seems limited to (probably) perf and tracing poking at user
> stack frames. But, yes, it definitely looks broken there.

So I ended up doing more cleanups - moving the 64-bit specific code to
'uaccess_64.h' etc.

And in the process I found another broken
thing:__untagged_addr_remote() is very very buggy.

The reason?

long sign = addr >> 63;

that does *not* do at all what '__untagged_addr()' does, because while
'sign' is a signed long, 'addr' is an *unsigned* long.

So the actual shift ends up being done as an unsigned shift, and then
just the result is assigned to a signed variable.

End result? 'sign' ends up being 0 for user space (intentional) or 1
for kernel space (not intentional)..

It's not 0 or ~0 as the __untagged_addr() is, wh9ch uses a signed
shift in the inline asm ('sar')

That said, while this is all horribly buggy, I'm not actually 100%
sure that we care too much about untagging kernel addresses. So it's
an error case that doesn't get marked as an error.

So I guess in *practice* the horribly buggy code is actually just a
small harmless bug, and causes anybody who passes in an invalid
(kernel) address look like a possibly valid user address after all.

HOWEVER.

Why does it do that "shift-by-63" game there, instead of making
tlbstate_untag_mask just have bit #63 always set?

Untagging a kernel address is not a sensible operation, so the only
thing you want is to *keep* a kernel address as a bad address. You
don't have to actually keep it a *valid* kernel address, it just
should not become a (possibly valid) user address after untagging.

Hmmm? Am I missing something?

Linus

2023-05-03 08:03:31

by Peter Zijlstra

[permalink] [raw]

Subject: Re: [GIT PULL] x86/mm for 6.4

On Tue, May 02, 2023 at 01:14:33PM -0700, Linus Torvalds wrote:
> On Tue, May 2, 2023 at 9:00 AM Linus Torvalds
> <[email protected]> wrote:
> >
> > > I guess it also wouldn't matter as much either if we hid it in a helper
> > > like the attached patch and I didn't have to read it twice. ;)
> >
> > Yeah, I think that's a good solution.
>
> Hmm. And as I was rebasing the patch to fix up my patch, I realized
> that the current -git top-of-tree state is actually broken.
>
> That
>
> #define access_ok(addr, size) \
> ({ \
> WARN_ON_IN_IRQ(); \
> likely(__access_ok(untagged_addr(addr), size)); \
> })
>
> is actually *wrong* in two ways.
>
> Now, in my original patch, I added a comment about how that
> "WARN_ON_IN_IRQ()" is bogus and this shouldn't be x86-specific at all.
>
> I ended up going back in time to see why it was added, and I think it
> was added because we used to access 'current' in access_ok(), due to
> it using that user_addr_max() thing:
>
> likely(!__range_not_ok(addr, size, user_addr_max()));
>
> but that was all removed by the set_fs() removal by Christoph Hellwig.

So I had a poke around, trying to figure out where it came from, and
yes. Commit ae31fe51a3cc ("perf/x86: Restore TASK_SIZE check on frame
pointer") is the reason that WARN_ON_IN_IRQ() thing got added.

2023-05-03 16:46:54

[permalink] [raw]

Subject: Re: [GIT PULL] x86/mm for 6.4

On Wed, May 3, 2023 at 1:01 AM Peter Zijlstra <[email protected]> wrote:
>
> So I had a poke around, trying to figure out where it came from, and
> yes. Commit ae31fe51a3cc ("perf/x86: Restore TASK_SIZE check on frame
> pointer") is the reason that WARN_ON_IN_IRQ() thing got added.

I added a pointer to that commit in the commit message, and took the
above to mean 'Ack' from you.

Linus

2023-05-03 16:52:51

[permalink] [raw]

Subject: Re: [GIT PULL] x86/mm for 6.4

On 5/3/23 09:38, Linus Torvalds wrote:
>> Why does it do that "shift-by-63" game there, instead of making
>> tlbstate_untag_mask just have bit #63 always set?
> And it turns out that bit #63 really _is_ always set, so I think the
> solution to this all is to remove the sign games in untag_addr()
> entirely.

Yes, there are only two possible values right now, both of which have
bit 63 set:

LAM off: mm->context.untag_mask = -1UL;
LAM on: mm->context.untag_mask = ~GENMASK(62, 57);

> Untagging a kernel address will "corrupt" it, but it will stay a
> kernel address (well, it will stay a "high bit set" address), which is
> all we care about anyway.
>
> If somebody actually tries to untag a kernel address, that would be a
> bug anyway, as far as I can tell.

Is it a bug? The do_madvise() path, for instance, is passing a value in
there that came right from userspace.

> So I'm going to just remove the 'sign' games entirely. They are
> completely broken in 'untagged_addr_remote()', they _used_ to be
> completely broken in 'untagged_addr()', and it looks like it's all
> unnecessary.

Yes, it looks completely superfluous.

2023-05-03 16:52:58

[permalink] [raw]

Subject: Re: [GIT PULL] x86/mm for 6.4

On Tue, May 2, 2023 at 6:17 PM Linus Torvalds
<[email protected]> wrote:
>
> And in the process I found another broken
> thing:__untagged_addr_remote() is very very buggy.
>
> The reason?
>
> long sign = addr >> 63;
>
> that does *not* do at all what '__untagged_addr()' does, because while
> 'sign' is a signed long, 'addr' is an *unsigned* long.
>
> So the actual shift ends up being done as an unsigned shift, and then
> just the result is assigned to a signed variable.
>
> End result? 'sign' ends up being 0 for user space (intentional) or 1
> for kernel space (not intentional)..

Looking around, this same bug used to exists for the normal
(non-remote) case too, until it was accidentally fixed when changing
that to use inline asm and the alternatives code.

At that point the non-remote case got an explicit 'sar' instruction,
and the result really was ~0 for kernel mode addresses.

> Why does it do that "shift-by-63" game there, instead of making
> tlbstate_untag_mask just have bit #63 always set?

And it turns out that bit #63 really _is_ always set, so I think the
solution to this all is to remove the sign games in untag_addr()
entirely.

Untagging a kernel address will "corrupt" it, but it will stay a
kernel address (well, it will stay a "high bit set" address), which is
all we care about anyway.

If somebody actually tries to untag a kernel address, that would be a
bug anyway, as far as I can tell.

So I'm going to just remove the 'sign' games entirely. They are
completely broken in 'untagged_addr_remote()', they _used_ to be
completely broken in 'untagged_addr()', and it looks like it's all
unnecessary.

Linus

2023-05-03 17:04:15

[permalink] [raw]

Subject: Re: [GIT PULL] x86/mm for 6.4

On Wed, May 3, 2023 at 9:45 AM Dave Hansen <[email protected]> wrote:
>
> On 5/3/23 09:38, Linus Torvalds wrote:
> > If somebody actually tries to untag a kernel address, that would be a
> > bug anyway, as far as I can tell.
>
> Is it a bug? The do_madvise() path, for instance, is passing a value in
> there that came right from userspace.

That's still a "user address" - just not a *valid* one.

So we do not want to mask the high bit off - because that is what will
catch people later doing things like vma address range comparisons on
it and notice "that's not a valid address", but it's also not a
"kernel address" that we need to preserve as such.

So yeah, it's a bit confusing in that it's _also_ true that "kernel
addresses have the high bit set" and "user addresses have the high bit
clear", and I'm basically using two different semantics for "kernel
address".

IOW: what I mean by "it's not valid to do 'untagged_addr()' on a
kernel address" is that you can't take a (valid) kernel address, do
'untagged_addr()' on it, and expect it to still work as a kernel
address.

But at the same time you *are* supposed to be able to use
'untagged_addr()' on a - untrusted and possibly invalid - user
pointer, and it's supposed to end up having the tag bits clear and
still be usable as a user pointer. And yet still also be caught by any
validity checks (ie a high bit set would never be a valid user
pointer, not even after doing 'untagged_addr()' on it).

Linus

2023-05-03 18:13:21

[permalink] [raw]

Subject: Re: [GIT PULL] x86/mm for 6.4

On Wed, May 3, 2023 at 9:38 AM Linus Torvalds
<[email protected]> wrote:
>
> So I'm going to just remove the 'sign' games entirely. They are
> completely broken in 'untagged_addr_remote()', they _used_ to be
> completely broken in 'untagged_addr()', and it looks like it's all
> unnecessary.

Ok, I've pushed out my changes to the 'x86-uaccess-cleanup' branch.

I think it's all good, but it would be really nice to hear it's been
tested on a setup that actually has LAM (simulator? or maybe there is
actual hw inside Intel)

And any other commentary is appreciated,

Linus

2023-05-03 19:29:02

[permalink] [raw]

Subject: Re: [GIT PULL] x86/mm for 6.4

On Wed, May 3, 2023 at 12:02 PM Peter Zijlstra <[email protected]> wrote:
>
> I suppose that's a very long winded way of saying, that yes, ofcourse 0
> is a positive number :-)

Well, I do consider it as such, but I guess I took all my math classes
when people still considered negative, zero and positive to be three
disjoint sets, and 'non-negative' was required for rigor.

Some googling around says that a lot of people still think that, and
that it might even be language-specific.

I think the commit commentary about "ok, strictly non-negative" might
still be relevant. At least to some people, and at least for
sticklers.

Also, I do consider 0 to be part of ℕ, although I wouldn't consider
that to be an argument about "positive" at all.

The argument for 0 in ℕ would be that without it, you don't have an
identity element for addition, which would arguably make natural
numbers kind of broken as a set.

Linus

2023-05-03 19:29:37

by Peter Zijlstra

[permalink] [raw]

Subject: Re: [GIT PULL] x86/mm for 6.4

On Wed, May 03, 2023 at 10:54:38AM -0700, Linus Torvalds wrote:
> On Wed, May 3, 2023 at 9:38 AM Linus Torvalds
> <[email protected]> wrote:
> >
> > So I'm going to just remove the 'sign' games entirely. They are
> > completely broken in 'untagged_addr_remote()', they _used_ to be
> > completely broken in 'untagged_addr()', and it looks like it's all
> > unnecessary.
>
> Ok, I've pushed out my changes to the 'x86-uaccess-cleanup' branch.
>
> I think it's all good, but it would be really nice to hear it's been
> tested on a setup that actually has LAM (simulator? or maybe there is
> actual hw inside Intel)
>
> And any other commentary is appreciated,

Looks sane from a first reading. But I'll try and have another look
tomorrow.

Also per how 2s complement is constructed 0 has to be in the positive
space for it to be 'half'. Also, typically 0 is included in N and the
traditional 'positive integers' set is then written as N \ {0}, but
that's somewhat modern and a lot of variation exists -- although
typically books tend to specify where they stand on that issue.

I suppose that's a very long winded way of saying, that yes, ofcourse 0
is a positive number :-)

2023-05-04 06:52:27

[permalink] [raw]

Subject: Re: [GIT PULL] x86/mm for 6.4

On Wed, May 03, 2023 at 09:38:03AM -0700, Linus Torvalds wrote:
> > Why does it do that "shift-by-63" game there, instead of making
> > tlbstate_untag_mask just have bit #63 always set?
>
> And it turns out that bit #63 really _is_ always set, so I think the
> solution to this all is to remove the sign games in untag_addr()
> entirely.

Untagging kernel pointer with LAM enabled will land it in the canonical
hole which is safe as it leads to #GP on dereference.

> Untagging a kernel address will "corrupt" it, but it will stay a
> kernel address (well, it will stay a "high bit set" address), which is
> all we care about anyway.

The interesting case to consider is untagging kernel pointer when LAM_U48
is enabled (not part of current LAM enabling). LAM_U48 would make the
untagging mask wider -- ~GENMASK(62, 48). With 5-level paging and LAM_SUP
enabled (also not supported yet) untagging kernel may transform it to
other valid kernel pointer.

So we cannot rely on #GP as backstop here. The kernel has to exclude
kernel pointer by other means. It can be fun to debug.

Looks like you are not worried about this kind of corruption. Hm.

Maybe it worth to indicate in the helper name that it is intended only for
userspace addresses? Rename it to untagged_uaddr() or something?

--
Kiryl Shutsemau / Kirill A. Shutemov

2023-05-04 15:37:46

[permalink] [raw]

Subject: Re: [GIT PULL] x86/mm for 6.4

On 5/3/23 23:28, Kirill A. Shutemov wrote:
>> Untagging a kernel address will "corrupt" it, but it will stay a
>> kernel address (well, it will stay a "high bit set" address), which is
>> all we care about anyway.
> The interesting case to consider is untagging kernel pointer when LAM_U48
> is enabled (not part of current LAM enabling). LAM_U48 would make the
> untagging mask wider -- ~GENMASK(62, 48). With 5-level paging and LAM_SUP
> enabled (also not supported yet) untagging kernel may transform it to
> other valid kernel pointer.
>
> So we cannot rely on #GP as backstop here. The kernel has to exclude
> kernel pointer by other means. It can be fun to debug.

Yeah, I have the feeling that we're really going to need a pair of
untagging functions once we get to doing kernel LAM for a _bunch_ of
reasons.

Just as a practical matter, I think we should OR bits into the mask on
the kernel side, effectively:

unsigned long untag_kernel_addr(unsigned long addr)
{
return addr | kernel_untag_mask;
}

and kernel_untag_mask should have bit 63 *clear*.

That way the pointers that have gone through untagging won't look silly.
If you untag VMALLOC_END or something, it'll still look like the
addresses we have in mm.rst.

Also, it'll be impossible to have the mask turn a userspace address into
a kernel one.

Last, we can add some debugging in there, probably conditional on some
mm debugging options like:

if (WARN_ON_ONCE(!valid_user_address(addr)))
return 0;

It's kinda like "void __user *" versus "void *". The __user ones can
*absolutely* point anywhere, user or kernel. That's why we can't WARN()
in the untagged_addr() function that takes user pointers.

But "void *" can only point to the kernel. It has totally different rules.

We should probably also do something like the attached patch sooner
rather than later. 'untag_mask' really is a misleading name for a mask
that gets applied only to user addresses.

Attachments:

lam.patch (6.78 kB)

2023-05-04 17:17:27

[permalink] [raw]

Subject: Re: [GIT PULL] x86/mm for 6.4

On Wed, May 3, 2023 at 11:28 PM Kirill A. Shutemov <[email protected]> wrote:
>
> On Wed, May 03, 2023 at 09:38:03AM -0700, Linus Torvalds wrote:
> > > Why does it do that "shift-by-63" game there, instead of making
> > > tlbstate_untag_mask just have bit #63 always set?
> >
> > And it turns out that bit #63 really _is_ always set, so I think the
> > solution to this all is to remove the sign games in untag_addr()
> > entirely.
>
> Untagging kernel pointer with LAM enabled will land it in the canonical
> hole which is safe as it leads to #GP on dereference.

You are entirely missing the point.

The GP fault does *NOT MATTER*.

Think of 'untagged_addr()' as a companion to - but absolutely *NOT* a
replacement for - 'access_ok()'.

You have three distinct cases:

(a) user-supplied valid user address

(b) user-supplied invalid user address (it high bit set)

(c) actual kernel address

and 'untagged_addr()' and 'access_ok()' work on the same basic input
domain: cases (a) and (b).

And the important thing for 'untagged_addr()' is that in case (a) it
needs to remove the tab bits, and in case (b) needs to *keep* the
address as an invalid user address.

Note that it does not need to keep the value the *same*. Nobody cares.

And also note that the resulting pointer may or may not GP-fault. Nobody cares.

Just to hit this home with a very explicit example, think of the case
where the kernel config for address masking isn't even enabled, and
'untagged_addr()' is a 1:1 map with no changes.

Doing 'untagged_addr()' on a valid user address results in a valid
user address. And doing it on an invalid user address results in an
invalid user address. GOOD.

Note that doing 'untagged_addr()' on an invalid user access does NOT
IN ANY WAY make that invalid address somehow "safe" and cause a GP
fault. Sure, it _might_ GP-fault. Or it might not. It might point to
random kernel data.

Verification of the address is simply not the job of
'untagged_addr()'. Never has been, never will be, and fundamentally
*cannot* be, since for the forseeable future 'untagged_addr()' is a
no-op for every single user outside of Intel.

Verification is separate. The verification is never "let's randomly
just access this pointer and see if it gets a GP-fault". No. We have a
user pointer, it needs *checking*. It needs to have something like
"use lookup_vma() to look up the vma that is associated with that
address". But it could also be something like "just do the range check
in GUP".

And that's why "keep an invalid user address as an invalid address",
because that *separate* stage of verifying the address needs to still
show that it's invalid.

Now, sometimes the "verification" might actually be "access_ok()"
itself, but honestly, if the address is used for an actual access,
then it shouldn't have gone through the 'untagged_addr()' thing at
all. It should just have been used as-is for the access. So normally
'untagged_addr()' does not get used *together* with 'access_ok()',
although that should obviously also work.

End result: all that really matters on x86-64 is that
'untagged_addr()' must keep the high bit as-is. That's the "this is
not a valid user address" bit. That's very much explicit in my current
series, of course, but even without my current series it was
implicitly the truth with those LAM patches (particularly considering
that 'untagged_addr()' didn't actually do the "keep kernel addresses
as-is" that it *thought* it did due to the signed type confusion).

So what about that (c) case? It doesn't matter. It's simply
fundamentally wrong for the kernel to pass an actual kernel address to
'untagged_addr()' and expect something useful back. It's a nonsensical
thing to do, and it's a huge bug.

So for the (c) case, the fact that the result would be useless and
*usually* GP-fault on access is a good thing. But it's not a
requirement, it's more of a "oh, cool, it's good that the nonsensical
operation causes quick failures".

So in that case, the GP fault is a "feature", but not a requirement.
Again, the normal 'untagged_addr()' case (of not changing the pointer
at all), obviously does *not* cause the kernel pointer corruption, but
maybe we could even have a debug mode. We could literally make
'untagged_addr()' do something like

#ifdef CONFIG_DEBUG_NONLAM
// Make sure nobody tries to use untagged_addr() on non-user addresses
#define untagged_addr(x) ((x) | (long)(x)>>63)
#endif

except obviously with the "get the types right and use 'x' only once"
thing (so the above #define is buggy, and puresly for conceptual
documentation purposes).

See?

Side note: one day, maybe we want to use address tagging *inside* the
kernel. However, that will not use 'untagged_addr()'. That would use
some *other* model for cleaning up kernel pointers when necessary.

Even on x86-64, the tagging rules for kernel and user space is
entirely different, in that user-space LAM rules are "U48" or "U57",
while kernel LAM rules depend on the paging mode, and the two are
*not* tied together.

But more importantly from a kernel perspective: regardless of hardware
implementations like that, the notion of masking bits off a untrusted
user pointer is simply completely *different* from the notion of
masking off bits of our own kernel pointers. You can see this in the
kernel everywhere: user pointers should be statically always user
pointers, and should look something like

struct iocb __user *user_iocb

which is very very different from a kernel pointer that doesn't have
that "__user" thing. Again, on some architectures, the *EXACT*SAME*
bit pattern pointer value may point to different things, because user
accesses are simply in a different address space entirely.

So if/when KASAN starts using LAM inside the kernel, it will do its
own things. It had better not use "untagged_addr()".

Linus

2023-05-04 17:36:45

[permalink] [raw]

Subject: Re: [GIT PULL] x86/mm for 6.4

On Thu, May 04, 2023 at 08:25:58AM -0700, Dave Hansen wrote:
> On 5/3/23 23:28, Kirill A. Shutemov wrote:
> >> Untagging a kernel address will "corrupt" it, but it will stay a
> >> kernel address (well, it will stay a "high bit set" address), which is
> >> all we care about anyway.
> > The interesting case to consider is untagging kernel pointer when LAM_U48
> > is enabled (not part of current LAM enabling). LAM_U48 would make the
> > untagging mask wider -- ~GENMASK(62, 48). With 5-level paging and LAM_SUP
> > enabled (also not supported yet) untagging kernel may transform it to
> > other valid kernel pointer.
> >
> > So we cannot rely on #GP as backstop here. The kernel has to exclude
> > kernel pointer by other means. It can be fun to debug.
>
> Yeah, I have the feeling that we're really going to need a pair of
> untagging functions once we get to doing kernel LAM for a _bunch_ of
> reasons.

There's already arch_kasan_reset_tag() used on ARM64 (and two more helpers
to set/get tag). Don't see a reason to add new.

> Just as a practical matter, I think we should OR bits into the mask on
> the kernel side, effectively:
>
> unsigned long untag_kernel_addr(unsigned long addr)
> {
> return addr | kernel_untag_mask;
> }
>
> and kernel_untag_mask should have bit 63 *clear*.
>
> That way the pointers that have gone through untagging won't look silly.
> If you untag VMALLOC_END or something, it'll still look like the
> addresses we have in mm.rst.
>
> Also, it'll be impossible to have the mask turn a userspace address into
> a kernel one.
>
> Last, we can add some debugging in there, probably conditional on some
> mm debugging options like:
>
> if (WARN_ON_ONCE(!valid_user_address(addr)))
> return 0;
>
> It's kinda like "void __user *" versus "void *". The __user ones can
> *absolutely* point anywhere, user or kernel. That's why we can't WARN()
> in the untagged_addr() function that takes user pointers.
>
> But "void *" can only point to the kernel. It has totally different rules.
>
> We should probably also do something like the attached patch sooner
> rather than later. 'untag_mask' really is a misleading name for a mask
> that gets applied only to user addresses.

A bit too verbose to my liking, but okay.

Maybe _uaddr() instead of _user_addr()?

--
Kiryl Shutsemau / Kirill A. Shutemov

2023-05-05 20:28:19

[permalink] [raw]

Subject: Re: [GIT PULL] x86/mm for 6.4

On Wed, May 3, 2023 at 12:02 PM Peter Zijlstra <[email protected]> wrote:
>
> Looks sane from a first reading. But I'll try and have another look
> tomorrow.

Well, I didn't hear back any more, and I was merging architecture
code, so I merged my branch too.

It does fix at least two bugs, even if those bugs are not necessarily
all that noticeable.

I guess we'll see if it introduces new ones...

Linus

2023-05-05 21:21:16

[permalink] [raw]

Subject: Re: [GIT PULL] x86/mm for 6.4

On Fri, May 05, 2023 at 12:56:31PM -0700, Linus Torvalds wrote:
> On Wed, May 3, 2023 at 12:02 PM Peter Zijlstra <[email protected]> wrote:
> >
> > Looks sane from a first reading. But I'll try and have another look
> > tomorrow.
>
> Well, I didn't hear back any more, and I was merging architecture
> code, so I merged my branch too.
>
> It does fix at least two bugs, even if those bugs are not necessarily
> all that noticeable.
>
> I guess we'll see if it introduces new ones...

The branch passed LAM selftests fine.

--
Kiryl Shutsemau / Kirill A. Shutemov

2023-06-16 08:59:51

by Alexander Potapenko

[permalink] [raw]

Subject: Re: [GIT PULL] x86/mm for 6.4

Hi Linus,

> static bool ex_handler_uaccess(const struct exception_table_entry *fixup,
> - struct pt_regs *regs, int trapnr)
> + struct pt_regs *regs, int trapnr,
> + unsigned long fault_address)
> {
> - WARN_ONCE(trapnr == X86_TRAP_GP, "General protection fault in user access. Non-canonical address?");
> + WARN_ONCE(trapnr == X86_TRAP_GP && !gp_fault_address_ok(fault_address),
> + "General protection fault in user access. Non-canonical address?");
> return ex_handler_default(fixup, regs);
> }

Shouldn't ex_handler_copy() be fixed in the same way?
Looks like it's still possible for a tagged userspace address to be passed to it and trigger a warning.

Alex

2023-06-16 16:44:41