linux-kernel - RE: [PATCH] net: untag pointer in sockptr_is

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: <1597223732.5467.10.camel@mtkswgap22>
Date:   Wed, 12 Aug 2020 17:15:32 +0800
From:   Miles Chen <miles.chen@...iatek.com>
To:     David Laight <David.Laight@...LAB.COM>
CC:     'Christoph Hellwig' <hch@....de>,
        "David S . Miller" <davem@...emloft.net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-mediatek@...ts.infradead.org" 
        <linux-mediatek@...ts.infradead.org>,
        "wsd_upstream@...iatek.com" <wsd_upstream@...iatek.com>
Subject: RE: [PATCH] net: untag pointer in sockptr_is_kernel

On Tue, 2020-08-11 at 11:44 +0000, David Laight wrote:
> > On Tue, Aug 11, 2020 at 06:27:04PM +0800, Miles Chen wrote:
> > > From: Miles Chen <miles.chen@...iatek.com>
> > >
> > > sockptr_is_kernel() uses (sockptr.kernel >= TASK_SIZE) to tell
> > > if the pointer is kernel space or user space. When user space uses
> > > the "top byte ignored" feature such as HWAsan, we must untag
> > > the pointer before checking against TASK_SIZE.
> > >
> > > sockptr_is_kernel() will view a tagged user pointer as a kernel pointer
> > > and use memcpy directly and causes a kernel crash.
> > 
> > Dave merged a patch from me to rever the optimized sockptr
> > implementation for now.  If we bring it back we should fold in your
> > fix.
> 
> I missed that going though :-(
> I've looked for a fix to the access_ok(kernel_addr,0) being true issue.
> 
> Shouldn't TASK_SIZE be increased to cover all the 'tagged' addresses?
> ISTR the 'tag' bits are the 'next' 8 (or so) address bits at the top
> of valid user addresses.


I'm not sure if this is a good idea. TASK_SIZE is an arch dependent
constant, if we increase TASK_SIZE to cover the 'tagged' address space,
the TASK_SIZE will not tell us the actual virtual address size.

Maybe we need a macro to tell if a pointer is in user space or not and
handle the memory tag there.
But this only works for the "is this pointer in user space" problem. 


I reported another tagged pointer issue [1].

Miles

[1] https://lore.kernel.org/patchwork/patch/1283649/



> Then presumably the user-copies would be able to use the tagged
> address values getting whatever protection that gives.
> 
> ISTM that for kernel-user boundary checks TASK_SIZE is the
> wrong constant.
> (The upper limit for mmap() is entirely different.)
> The limit should be independent of whether the process is 32 or 64bit
> (any fault above 4G will fail to find a user-page for 32bit).
> The limit can also go well into the address 'black hole' that
> exists on x86-x64 (and similar) between valid user and kernel
> addresses - handling the relevant trap should be too hard
> (it is always an error, so need not be fast).
> 
> So with set_fs(KERNEL_DS) gone x86-x64 can (almost certainly)
> do a cheap test for (long)addr >= 0 in access_ok() (+ length test).
> While set_fs() is needed it can be:
> 	((long)addr & current->mask) >= 0
> (masking off the top bit if kernel addresses are valid).
> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>