| Message ID | 20200728063643.396100-5-hch@lst.de |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | sockptr_t fixes | expand |
On Tue, 28 Jul 2020 08:36:43 +0200 Christoph Hellwig wrote: > Make sure not just the pointer itself but the whole range lies in > the user address space. For that pass the length and then use > the access_ok helper to do the check. > > Fixes: 6d04fe15f78a ("net: optimize the sockptr_t for unified kernel/user address spaces") > Reported-by: David Laight <David.Laight@ACULAB.COM> > Signed-off-by: Christoph Hellwig <hch@lst.de> > diff --git a/net/ipv4/bpfilter/sockopt.c b/net/ipv4/bpfilter/sockopt.c > index 94f18d2352d007..8b132c52045973 100644 > --- a/net/ipv4/bpfilter/sockopt.c > +++ b/net/ipv4/bpfilter/sockopt.c > @@ -65,7 +65,7 @@ int bpfilter_ip_get_sockopt(struct sock *sk, int optname, > > if (get_user(len, optlen)) > return -EFAULT; > - err = init_user_sockptr(&optval, user_optval); > + err = init_user_sockptr(&optval, user_optval, *optlen); > if (err) > return err; > return bpfilter_mbox_request(sk, optname, optval, len, false); Appears to cause these two new warnings, sadly: net/ipv4/bpfilter/sockopt.c:68:56: warning: dereference of noderef expression net/ipv4/bpfilter/sockopt.c:68:56: warning: dereference of noderef expression
On Tue, Jul 28, 2020 at 08:47:46AM -0700, Jakub Kicinski wrote: > On Tue, 28 Jul 2020 08:36:43 +0200 Christoph Hellwig wrote: > > Make sure not just the pointer itself but the whole range lies in > > the user address space. For that pass the length and then use > > the access_ok helper to do the check. > > > > Fixes: 6d04fe15f78a ("net: optimize the sockptr_t for unified kernel/user address spaces") > > Reported-by: David Laight <David.Laight@ACULAB.COM> > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > > diff --git a/net/ipv4/bpfilter/sockopt.c b/net/ipv4/bpfilter/sockopt.c > > index 94f18d2352d007..8b132c52045973 100644 > > --- a/net/ipv4/bpfilter/sockopt.c > > +++ b/net/ipv4/bpfilter/sockopt.c > > @@ -65,7 +65,7 @@ int bpfilter_ip_get_sockopt(struct sock *sk, int optname, > > > > if (get_user(len, optlen)) > > return -EFAULT; > > - err = init_user_sockptr(&optval, user_optval); > > + err = init_user_sockptr(&optval, user_optval, *optlen); > > if (err) > > return err; > > return bpfilter_mbox_request(sk, optname, optval, len, false); > > Appears to cause these two new warnings, sadly: > > net/ipv4/bpfilter/sockopt.c:68:56: warning: dereference of noderef expression > net/ipv4/bpfilter/sockopt.c:68:56: warning: dereference of noderef expression Shouldn't this just be one? That one is justified, though as *optlen should be len.
From: Jakub Kicinski > Sent: 28 July 2020 16:48 > On Tue, 28 Jul 2020 08:36:43 +0200 Christoph Hellwig wrote: > > Make sure not just the pointer itself but the whole range lies in > > the user address space. For that pass the length and then use > > the access_ok helper to do the check. > > > > Fixes: 6d04fe15f78a ("net: optimize the sockptr_t for unified kernel/user address spaces") > > Reported-by: David Laight <David.Laight@ACULAB.COM> > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > > diff --git a/net/ipv4/bpfilter/sockopt.c b/net/ipv4/bpfilter/sockopt.c > > index 94f18d2352d007..8b132c52045973 100644 > > --- a/net/ipv4/bpfilter/sockopt.c > > +++ b/net/ipv4/bpfilter/sockopt.c > > @@ -65,7 +65,7 @@ int bpfilter_ip_get_sockopt(struct sock *sk, int optname, > > > > if (get_user(len, optlen)) > > return -EFAULT; > > - err = init_user_sockptr(&optval, user_optval); > > + err = init_user_sockptr(&optval, user_optval, *optlen); > > if (err) > > return err; > > return bpfilter_mbox_request(sk, optname, optval, len, false); > > Appears to cause these two new warnings, sadly: > > net/ipv4/bpfilter/sockopt.c:68:56: warning: dereference of noderef expression > net/ipv4/bpfilter/sockopt.c:68:56: warning: dereference of noderef expression Not surprising - 'optlen' is a user pointer. It should be passing 'len'. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Tue, 28 Jul 2020 17:51:40 +0200 Christoph Hellwig wrote: > On Tue, Jul 28, 2020 at 08:47:46AM -0700, Jakub Kicinski wrote: > > Appears to cause these two new warnings, sadly: > > > > net/ipv4/bpfilter/sockopt.c:68:56: warning: dereference of noderef expression > > net/ipv4/bpfilter/sockopt.c:68:56: warning: dereference of noderef expression > > Shouldn't this just be one? That one is justified, though as *optlen > should be len. Could be, seems like sparse duplicates some warnings in my build system, not sure why :S
From: Christoph Hellwig <hch@lst.de> Date: Tue, 28 Jul 2020 08:36:43 +0200 > if (get_user(len, optlen)) > return -EFAULT; ^^^^^^^^^^^^^^^^^^^^^^^^^^ > - err = init_user_sockptr(&optval, user_optval); > + err = init_user_sockptr(&optval, user_optval, *optlen); ^^^^^^^^ Hmmm?
On Tue, Jul 28, 2020 at 01:01:11PM -0700, David Miller wrote: > From: Christoph Hellwig <hch@lst.de> > Date: Tue, 28 Jul 2020 08:36:43 +0200 > > > if (get_user(len, optlen)) > > return -EFAULT; > ^^^^^^^^^^^^^^^^^^^^^^^^^^ > > - err = init_user_sockptr(&optval, user_optval); > > + err = init_user_sockptr(&optval, user_optval, *optlen); > ^^^^^^^^ > > Hmmm? A fixed version was already posted yesterday: https://lore.kernel.org/netdev/20200728163836.562074-1-hch@lst.de/T/#me3e9c7b71e39b4689628ed2e61dec06705344847
diff --git a/include/linux/sockptr.h b/include/linux/sockptr.h index 9e6c81d474cba8..96840def9d69cc 100644 --- a/include/linux/sockptr.h +++ b/include/linux/sockptr.h @@ -27,14 +27,6 @@ static inline sockptr_t KERNEL_SOCKPTR(void *p) { return (sockptr_t) { .kernel = p }; } - -static inline int __must_check init_user_sockptr(sockptr_t *sp, void __user *p) -{ - if ((unsigned long)p >= TASK_SIZE) - return -EFAULT; - sp->user = p; - return 0; -} #else /* CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE */ typedef struct { union { @@ -53,14 +45,16 @@ static inline sockptr_t KERNEL_SOCKPTR(void *p) { return (sockptr_t) { .kernel = p, .is_kernel = true }; } +#endif /* CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE */ -static inline int __must_check init_user_sockptr(sockptr_t *sp, void __user *p) +static inline int __must_check init_user_sockptr(sockptr_t *sp, void __user *p, + size_t size) { - sp->user = p; - sp->is_kernel = false; + if (!access_ok(p, size)) + return -EFAULT; + *sp = (sockptr_t) { .user = p }; return 0; } -#endif /* CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE */ static inline bool sockptr_is_null(sockptr_t sockptr) { diff --git a/net/ipv4/bpfilter/sockopt.c b/net/ipv4/bpfilter/sockopt.c index 94f18d2352d007..8b132c52045973 100644 --- a/net/ipv4/bpfilter/sockopt.c +++ b/net/ipv4/bpfilter/sockopt.c @@ -65,7 +65,7 @@ int bpfilter_ip_get_sockopt(struct sock *sk, int optname, if (get_user(len, optlen)) return -EFAULT; - err = init_user_sockptr(&optval, user_optval); + err = init_user_sockptr(&optval, user_optval, *optlen); if (err) return err; return bpfilter_mbox_request(sk, optname, optval, len, false); diff --git a/net/socket.c b/net/socket.c index 94ca4547cd7c53..aff52e81653ce3 100644 --- a/net/socket.c +++ b/net/socket.c @@ -2105,7 +2105,7 @@ int __sys_setsockopt(int fd, int level, int optname, char __user *user_optval, if (optlen < 0) return -EINVAL; - err = init_user_sockptr(&optval, user_optval); + err = init_user_sockptr(&optval, user_optval, optlen); if (err) return err;
Make sure not just the pointer itself but the whole range lies in the user address space. For that pass the length and then use the access_ok helper to do the check. Fixes: 6d04fe15f78a ("net: optimize the sockptr_t for unified kernel/user address spaces") Reported-by: David Laight <David.Laight@ACULAB.COM> Signed-off-by: Christoph Hellwig <hch@lst.de> --- include/linux/sockptr.h | 18 ++++++------------ net/ipv4/bpfilter/sockopt.c | 2 +- net/socket.c | 2 +- 3 files changed, 8 insertions(+), 14 deletions(-)