linux-kernel - Re: [PATCH] x86: predict __access

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGudoHFFr6vWaUocAbQhhq23Hh4r4R+b2vDvA0EJmRq9nxCO+g@mail.gmail.com>
Date: Tue, 1 Apr 2025 23:11:28 +0200
From: Mateusz Guzik <mjguzik@...il.com>
To: Ingo Molnar <mingo@...nel.org>
Cc: mingo@...hat.com, x86@...nel.org, linux-kernel@...r.kernel.org, 
	Linus Torvalds <torvalds@...ux-foundation.org>, Peter Zijlstra <peterz@...radead.org>, 
	Uros Bizjak <ubizjak@...il.com>, "H. Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH] x86: predict __access_ok() returning true

On Tue, Apr 1, 2025 at 11:00 PM Ingo Molnar <mingo@...nel.org> wrote:
>
>
> * Mateusz Guzik <mjguzik@...il.com> wrote:
>
> > On Tue, Apr 1, 2025 at 10:43 PM Ingo Molnar <mingo@...nel.org> wrote:
> > >
> > >
> > > * Ingo Molnar <mingo@...nel.org> wrote:
> > >
> > > > It's also the right place to have the hint: that user addresses are
> > > > valid is the common case we optimize for.
> > > >
> > > > Thanks,
> > > >
> > > >       Ingo
> > > >
> > > >  arch/x86/include/asm/uaccess_64.h | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h
> > > > index c52f0133425b..4c13883371aa 100644
> > > > --- a/arch/x86/include/asm/uaccess_64.h
> > > > +++ b/arch/x86/include/asm/uaccess_64.h
> > > > @@ -54,7 +54,7 @@ static inline unsigned long __untagged_addr_remote(struct mm_struct *mm,
> > > >  #endif
> > > >
> > > >  #define valid_user_address(x) \
> > > > -     ((__force unsigned long)(x) <= runtime_const_ptr(USER_PTR_MAX))
> > > > +     likely((__force unsigned long)(x) <= runtime_const_ptr(USER_PTR_MAX))
> > >
> > > Should we go this way, this is the safe macro variant:
> > >
> > >    #define valid_user_address(x) \
> > >         (likely((__force unsigned long)(x) <= runtime_const_ptr(USER_PTR_MAX)))
> > >
> >
> > Note the are 2 tests and the other one does not get covered by *this* likely:
> > valid_user_address(sum) && sum >= (__force unsigned long)ptr;
> >
> > as in sum >= ptr is left be.
> >
> > However, I confirmed that with your patch the issue also goes away so
> > I guess it is fine.
> >
> > I think it would be the safest to likely within valid_user_address()
> > like in your patch, and likely on the entire expression like in mine.
> >
> > That said, there will be no hard feelz if you just commit your patch
> > and drop mine.
>
> Feel free to turn it into a Co-developed-by patch:
>
>   From: Mateusz Guzik <mjguzik@...il.com>
>   ...
>
>   Co-developed-by: Ingo Molnar <mingo@...nel.org>
>   Signed-off-by: Ingo Molnar <mingo@...nel.org>
>   Signed-off-by: Mateusz Guzik <mjguzik@...il.com>
>
> Because all I did was to transform your fix into something a bit more
> maintainable. I didn't even test it.
>

This is 2 lines changed in total, both trivially.

I think it is going to look rather silly to have this "co-developed",
but I can do it if you insist.

My suggestion would be that you commit your thing and add:
Reported-and-tested-by: Mateusz Guzik <mjguzik@...il.com>

feel free to steal the disasm before/after
-- 
Mateusz Guzik <mjguzik gmail.com>