Re: [PATCH] net: untag pointer in sockptr_is_kernel

Re: [PATCH] net: untag pointer in sockptr_is_kernel

[Christoph Hellwig]

On Tue, Aug 11, 2020 at 06:27:04PM +0800, Miles Chen wrote:
> From: Miles Chen <miles.chen@mediatek.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.

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

RE: [PATCH] net: untag pointer in sockptr_is_kernel

[David Laight]

> On Tue, Aug 11, 2020 at 06:27:04PM +0800, Miles Chen wrote:
> > From: Miles Chen <miles.chen@mediatek.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.

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)


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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@mediatek.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)
> 

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

RE: [PATCH] net: untag pointer in sockptr_is_kernel

[David Laight]

From: Miles Chen <miles.chen@mediatek.com>
> Sent: 12 August 2020 10:16
> 
> 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@mediatek.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.

Well TASK_SIZE isn't a constant, it is read out of 'current'.
Typically it isn't even the limit on the address space since
(at least in x86) it is changed by setfs(KERNEL_DS) so that
access_ok() doesn't reject kernel addresses.

It is almost as if TASK_SIZE is only actually valid for the
check in access_ok().

I'd also have thought you'd want the kernel to use the 'tagged'
address so that the hardware checks it matches.
Or is there some scheme for having userpace use tagged pointers
without hardware support - but wouldn't that require masking
and/or large offsets all over the user code?

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH] net: untag pointer in sockptr_is_kernel

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3575 bytes --]

Hi Miles,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]
[also build test ERROR on linus/master]
[cannot apply to net/master hch-configfs/for-next sparc-next/master v5.8 next-20200811]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Miles-Chen/net-untag-pointer-in-sockptr_is_kernel/20200811-183033
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git bfdd5aaa54b0a44d9df550fe4c9db7e1470a11b8
config: x86_64-randconfig-s022-20200811 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.2-168-g9554805c-dirty
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/net.h:24,
                    from net/core/stream.c:18:
   include/linux/sockptr.h: In function 'sockptr_is_kernel':
>> include/linux/sockptr.h:23:24: error: implicit declaration of function 'untagged_addr' [-Werror=implicit-function-declaration]
      23 |  return (unsigned long)untagged_addr(sockptr.kernel) >= TASK_SIZE;
         |                        ^~~~~~~~~~~~~
   cc1: some warnings being treated as errors
--
   In file included from include/linux/net.h:24,
                    from net/ipv6/ip6_fib.c:20:
   include/linux/sockptr.h: In function 'sockptr_is_kernel':
>> include/linux/sockptr.h:23:24: error: implicit declaration of function 'untagged_addr' [-Werror=implicit-function-declaration]
      23 |  return (unsigned long)untagged_addr(sockptr.kernel) >= TASK_SIZE;
         |                        ^~~~~~~~~~~~~
   net/ipv6/ip6_fib.c: In function 'fib6_add':
   net/ipv6/ip6_fib.c:1373:25: warning: variable 'pn' set but not used [-Wunused-but-set-variable]
    1373 |  struct fib6_node *fn, *pn = NULL;
         |                         ^~
   cc1: some warnings being treated as errors
--
   In file included from include/linux/net.h:24,
                    from net/ipv6/udp.c:24:
   include/linux/sockptr.h: In function 'sockptr_is_kernel':
>> include/linux/sockptr.h:23:24: error: implicit declaration of function 'untagged_addr' [-Werror=implicit-function-declaration]
      23 |  return (unsigned long)untagged_addr(sockptr.kernel) >= TASK_SIZE;
         |                        ^~~~~~~~~~~~~
   net/ipv6/udp.c: At top level:
   net/ipv6/udp.c:1029:30: warning: no previous prototype for 'udp_v6_early_demux' [-Wmissing-prototypes]
    1029 | INDIRECT_CALLABLE_SCOPE void udp_v6_early_demux(struct sk_buff *skb)
         |                              ^~~~~~~~~~~~~~~~~~
   net/ipv6/udp.c:1070:29: warning: no previous prototype for 'udpv6_rcv' [-Wmissing-prototypes]
    1070 | INDIRECT_CALLABLE_SCOPE int udpv6_rcv(struct sk_buff *skb)
         |                             ^~~~~~~~~
   cc1: some warnings being treated as errors

vim +/untagged_addr +23 include/linux/sockptr.h

    20	
    21	static inline bool sockptr_is_kernel(sockptr_t sockptr)
    22	{
  > 23		return (unsigned long)untagged_addr(sockptr.kernel) >= TASK_SIZE;
    24	}
    25	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 33859 bytes --]

[-- Attachment #3: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH] net: untag pointer in sockptr_is_kernel

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3410 bytes --]

Hi Miles,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]
[also build test ERROR on linus/master]
[cannot apply to net/master hch-configfs/for-next sparc-next/master v5.8 next-20200811]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Miles-Chen/net-untag-pointer-in-sockptr_is_kernel/20200811-183033
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git bfdd5aaa54b0a44d9df550fe4c9db7e1470a11b8
config: x86_64-randconfig-a013-20200811 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 4f2ad15db535873dda9bfe248a2771023b64a43c)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from net/ipv6/af_inet6.c:29:
   In file included from include/linux/net.h:24:
>> include/linux/sockptr.h:23:24: error: implicit declaration of function 'untagged_addr' [-Werror,-Wimplicit-function-declaration]
           return (unsigned long)untagged_addr(sockptr.kernel) >= TASK_SIZE;
                                 ^
   1 error generated.
--
   In file included from net/ipv6/udp.c:24:
   In file included from include/linux/net.h:24:
>> include/linux/sockptr.h:23:24: error: implicit declaration of function 'untagged_addr' [-Werror,-Wimplicit-function-declaration]
           return (unsigned long)untagged_addr(sockptr.kernel) >= TASK_SIZE;
                                 ^
   net/ipv6/udp.c:1029:30: warning: no previous prototype for function 'udp_v6_early_demux' [-Wmissing-prototypes]
   INDIRECT_CALLABLE_SCOPE void udp_v6_early_demux(struct sk_buff *skb)
                                ^
   net/ipv6/udp.c:1029:25: note: declare 'static' if the function is not intended to be used outside of this translation unit
   INDIRECT_CALLABLE_SCOPE void udp_v6_early_demux(struct sk_buff *skb)
                           ^
                           static 
   net/ipv6/udp.c:1070:29: warning: no previous prototype for function 'udpv6_rcv' [-Wmissing-prototypes]
   INDIRECT_CALLABLE_SCOPE int udpv6_rcv(struct sk_buff *skb)
                               ^
   net/ipv6/udp.c:1070:25: note: declare 'static' if the function is not intended to be used outside of this translation unit
   INDIRECT_CALLABLE_SCOPE int udpv6_rcv(struct sk_buff *skb)
                           ^
                           static 
   2 warnings and 1 error generated.

vim +/untagged_addr +23 include/linux/sockptr.h

    20	
    21	static inline bool sockptr_is_kernel(sockptr_t sockptr)
    22	{
  > 23		return (unsigned long)untagged_addr(sockptr.kernel) >= TASK_SIZE;
    24	}
    25	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 35476 bytes --]

[-- Attachment #3: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH] net: untag pointer in sockptr_is_kernel

[Miles Chen]

On Tue, 2020-08-11 at 20:28 +0800, kernel test robot wrote:
> Hi Miles,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on net-next/master]
> [also build test ERROR on linus/master]
> [cannot apply to net/master hch-configfs/for-next sparc-next/master v5.8 next-20200811]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]

hi test robot,

thanks for the report.

6d04fe15f78a ("net: optimize the sockptr_t for unified kernel/user
address spaces") has been reverted, so I will not sent patch v2 for this
build error.

Miles

> url:    https://github.com/0day-ci/linux/commits/Miles-Chen/net-untag-pointer-in-sockptr_is_kernel/20200811-18303
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git bfdd5aaa54b0a44d9df550fe4c9db7e1470a11b8
> config: x86_64-randconfig-a013-20200811 (attached as .config)
> compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 4f2ad15db535873dda9bfe248a2771023b64a43c)
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # install x86_64 cross compiling tool for clang build
>         # apt-get install binutils-x86-64-linux-gnu
>         # save the attached .config to linux build tree
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
>    In file included from net/ipv6/af_inet6.c:29:
>    In file included from include/linux/net.h:24:
> >> include/linux/sockptr.h:23:24: error: implicit declaration of function 'untagged_addr' [-Werror,-Wimplicit-function-declaration]
>            return (unsigned long)untagged_addr(sockptr.kernel) >= TASK_SIZE;
>                                  ^
>    1 error generated.
> --
>    In file included from net/ipv6/udp.c:24:
>    In file included from include/linux/net.h:24:
> >> include/linux/sockptr.h:23:24: error: implicit declaration of function 'untagged_addr' [-Werror,-Wimplicit-function-declaration]
>            return (unsigned long)untagged_addr(sockptr.kernel) >= TASK_SIZE;
>                                  ^
>    net/ipv6/udp.c:1029:30: warning: no previous prototype for function 'udp_v6_early_demux' [-Wmissing-prototypes]
>    INDIRECT_CALLABLE_SCOPE void udp_v6_early_demux(struct sk_buff *skb)
>                                 ^
>    net/ipv6/udp.c:1029:25: note: declare 'static' if the function is not intended to be used outside of this translation unit
>    INDIRECT_CALLABLE_SCOPE void udp_v6_early_demux(struct sk_buff *skb)
>                            ^
>                            static 
>    net/ipv6/udp.c:1070:29: warning: no previous prototype for function 'udpv6_rcv' [-Wmissing-prototypes]
>    INDIRECT_CALLABLE_SCOPE int udpv6_rcv(struct sk_buff *skb)
>                                ^
>    net/ipv6/udp.c:1070:25: note: declare 'static' if the function is not intended to be used outside of this translation unit
>    INDIRECT_CALLABLE_SCOPE int udpv6_rcv(struct sk_buff *skb)
>                            ^
>                            static 
>    2 warnings and 1 error generated.
> 
> vim +/untagged_addr +23 include/linux/sockptr.h
> 
>     20	
>     21	static inline bool sockptr_is_kernel(sockptr_t sockptr)
>     22	{
>   > 23		return (unsigned long)untagged_addr(sockptr.kernel) >= TASK_SIZE;
>     24	}
>     25	
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek