RE: [PATCH] net: untag pointer in sockptr_is_kernel

[Miles Chen]

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@xxxxxxxxxxxx>
> > >
> > > 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)
>