From: Miles Chen <[email protected]>
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.
static inline int copy_from_sockptr_offset(void *dst, sockptr_t src,
size_t offset, size_t size)
{
if (!sockptr_is_kernel(src))
return copy_from_user(dst, src.user + offset, size);
memcpy(dst, src.kernel + offset, size);
return 0;
}
Crash log:
[ 26.066275] Unable to handle kernel access to user memory outside uaccess routines at virtual address 0000007b410f6990
[ 26.067640] Mem abort info:
[ 26.068004] ESR = 0x9600000f
[ 26.068400] EC = 0x25: DABT (current EL), IL = 32 bits
[ 26.084406] SET = 0, FnV = 0
[ 26.084805] EA = 0, S1PTW = 0
[ 26.085310] Data abort info:
[ 26.085686] ISV = 0, ISS = 0x0000000f
[ 26.086179] CM = 0, WnR = 0
[ 26.086565] user pgtable: 4k pages, 39-bit VAs, pgdp=00000001feb7c000
[ 26.087386] [0000007b410f6990] pgd=0000000201ac2003, p4d=0000000201ac2003, pud=0000000201ac2003, pmd=00000001fd58a003, pte=00e80001fca7bf53
[ 26.089111] Internal error: Oops: 9600000f [#1] PREEMPT SMP
[ 30.435941] pstate: 80400005 (Nzcv daif +PAN -UAO BTYPE=--)
[ 30.435950] pc : __memcpy+0xbc/0x180
[ 30.435956] lr : copy_from_sockptr_offset+0x120/0x298
[ 30.435960] sp : ffffffc0179438d0
[ 30.441225] x29: ffffffc0179438d0 x28: ffffff81bf579940
[ 30.441898] x27: ffffffd24a752bc0 x26: ffffffc0179439b0
[ 30.442572] x25: ffffffd24a75dd60 x24: 0000000000000002
[ 30.443247] x23: ffffff81bf579940 x22: 0000000000000000
[ 30.443919] x21: b400007a2e8749f0 x20: ffffffc0179439b0
[ 30.444592] x19: 0000000000000060 x18: 0000000000000000
[ 30.445264] x17: 0000000000000000 x16: 0000000000000000
[ 30.445937] x15: 0000000000000552 x14: 00000000ba02a83b
[ 30.446609] x13: 0000000045fb4ba9 x12: 00000000e5d1c812
[ 30.447281] x11: 0000000000019cde x10: 0000000000000001
[ 30.447952] x9 : 00000000fffff000 x8 : 0000008000000000
[ 30.448625] x7 : ffffffd24932449c x6 : ffffffc0179439b0
[ 30.449295] x5 : 0000000000000000 x4 : 0000000000000000
[ 30.449967] x3 : 0000000000000060 x2 : ffffffffffffffe0
[ 30.450639] x1 : b400007a2e8749f0 x0 : ffffffc0179439b0
[ 30.451310] Call trace:
[ 30.451628] __memcpy+0xbc/0x180
[ 30.452039] do_ipt_set_ctl+0x88/0xa48
[ 30.452520] nf_setsockopt+0xc4/0x104
[ 30.452988] ip_setsockopt+0x128/0xfa8
[ 30.453466] raw_setsockopt+0x48/0x2c0
[ 30.453944] sock_common_setsockopt+0x18/0x20
[ 30.454497] __sys_setsockopt+0x144/0x1ec
[ 30.455005] __arm64_sys_setsockopt+0x24/0x30
[ 30.455560] el0_svc_common+0xa0/0x18c
[ 30.456039] do_el0_svc+0x78/0x80
[ 30.456462] el0_sync_handler+0xec/0x2b8
[ 30.456960] el0_sync+0x144/0x180
[ 30.457386] Code: 380014c3 14000028 f1020042 5400024a (a8c12027)
[ 30.458156] ---[ end trace a53f2349dbbd41cd ]---
[ 30.465290] Kernel panic - not syncing: Fatal exception
Signed-off-by: Miles Chen <[email protected]>
Fixes: 6d04fe15f78a ("net: optimize the sockptr_t for unified kernel/user address spaces")
---
include/linux/sockptr.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/sockptr.h b/include/linux/sockptr.h
index 96840def9d69..3febc4da1056 100644
--- a/include/linux/sockptr.h
+++ b/include/linux/sockptr.h
@@ -20,7 +20,7 @@ typedef union {
static inline bool sockptr_is_kernel(sockptr_t sockptr)
{
- return (unsigned long)sockptr.kernel >= TASK_SIZE;
+ return (unsigned long)untagged_addr(sockptr.kernel) >= TASK_SIZE;
}
static inline sockptr_t KERNEL_SOCKPTR(void *p)
--
2.18.0
On Tue, Aug 11, 2020 at 06:27:04PM +0800, Miles Chen wrote:
> From: Miles Chen <[email protected]>
>
> 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.
> On Tue, Aug 11, 2020 at 06:27:04PM +0800, Miles Chen wrote:
> > From: Miles Chen <[email protected]>
> >
> > 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)
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 <[email protected]>
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/[email protected]
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 <[email protected]>
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/[email protected]
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 <[email protected]>
> > >
> > > 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)
>
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 <[email protected]>
>
> 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/[email protected]
From: Miles Chen <[email protected]>
> 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 <[email protected]>
> > > >
> > > > 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)