linux-kernel - Re: [PATCH v4 32/35] s390/uaccess: Add KMSAN support to put_user() and get

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]

Date: Tue, 18 Jun 2024 11:56:06 +0200
From: Ilya Leoshkevich <iii@...ux.ibm.com>
To: Alexander Potapenko <glider@...gle.com>
Cc: Alexander Gordeev <agordeev@...ux.ibm.com>,
        Andrew Morton
 <akpm@...ux-foundation.org>,
        Christoph Lameter <cl@...ux.com>,
        David
 Rientjes <rientjes@...gle.com>,
        Heiko Carstens <hca@...ux.ibm.com>,
        Joonsoo
 Kim <iamjoonsoo.kim@....com>, Marco Elver <elver@...gle.com>,
        Masami
 Hiramatsu <mhiramat@...nel.org>,
        Pekka Enberg <penberg@...nel.org>,
        Steven
 Rostedt <rostedt@...dmis.org>,
        Vasily Gorbik <gor@...ux.ibm.com>, Vlastimil
 Babka <vbabka@...e.cz>,
        Christian Borntraeger <borntraeger@...ux.ibm.com>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Hyeonggon Yoo <42.hyeyoo@...il.com>, kasan-dev@...glegroups.com,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        linux-s390@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
        Mark Rutland <mark.rutland@....com>,
        Roman Gushchin <roman.gushchin@...ux.dev>,
        Sven Schnelle
 <svens@...ux.ibm.com>
Subject: Re: [PATCH v4 32/35] s390/uaccess: Add KMSAN support to put_user()
 and get_user()

On Tue, 2024-06-18 at 11:52 +0200, Alexander Potapenko wrote:
> On Tue, Jun 18, 2024 at 11:40 AM Ilya Leoshkevich <iii@...ux.ibm.com>
> wrote:
> > 
> > On Tue, 2024-06-18 at 11:24 +0200, Alexander Potapenko wrote:
> > > On Thu, Jun 13, 2024 at 5:39 PM Ilya Leoshkevich
> > > <iii@...ux.ibm.com>
> > > wrote:
> > > > 
> > > > put_user() uses inline assembly with precise constraints, so
> > > > Clang
> > > > is
> > > > in principle capable of instrumenting it automatically.
> > > > Unfortunately,
> > > > one of the constraints contains a dereferenced user pointer,
> > > > and
> > > > Clang
> > > > does not currently distinguish user and kernel pointers.
> > > > Therefore
> > > > KMSAN attempts to access shadow for user pointers, which is not
> > > > a
> > > > right
> > > > thing to do.
> > > > 
> > > > An obvious fix to add __no_sanitize_memory to __put_user_fn()
> > > > does
> > > > not
> > > > work, since it's __always_inline. And __always_inline cannot be
> > > > removed
> > > > due to the __put_user_bad() trick.
> > > > 
> > > > A different obvious fix of using the "a" instead of the "+Q"
> > > > constraint
> > > > degrades the code quality, which is very important here, since
> > > > it's
> > > > a
> > > > hot path.
> > > > 
> > > > Instead, repurpose the __put_user_asm() macro to define
> > > > __put_user_{char,short,int,long}_noinstr() functions and mark
> > > > them
> > > > with
> > > > __no_sanitize_memory. For the non-KMSAN builds make them
> > > > __always_inline in order to keep the generated code quality.
> > > > Also
> > > > define __put_user_{char,short,int,long}() functions, which call
> > > > the
> > > > aforementioned ones and which *are* instrumented, because they
> > > > call
> > > > KMSAN hooks, which may be implemented as macros.
> > > 
> > > I am not really familiar with s390 assembly, but I think you
> > > still
> > > need to call kmsan_copy_to_user() and kmsan_copy_from_user() to
> > > properly initialize the copied data and report infoleaks.
> > > Would it be possible to insert calls to linux/instrumented.h
> > > hooks
> > > into uaccess functions?
> > 
> > Aren't the existing instrument_get_user() / instrument_put_user()
> > calls
> > sufficient?
> 
> Oh, sorry, I overlooked them. Yes, those should be sufficient.
> But you don't include linux/instrumented.h, do you?

No, apparently we get this include from somewhere else by accident.
I will add it in a separate patch.