| Message ID | 20200723060908.50081-13-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/26] bpfilter: fix up a sparse annotation | expand |
Hi Christoph, On Thu, Jul 23, 2020 at 08:08:54AM +0200, Christoph Hellwig wrote: > diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c > index da933f99b5d517..42befbf12846c0 100644 > --- a/net/ipv4/ip_sockglue.c > +++ b/net/ipv4/ip_sockglue.c > @@ -1422,7 +1422,8 @@ int ip_setsockopt(struct sock *sk, int level, > optname != IP_IPSEC_POLICY && > optname != IP_XFRM_POLICY && > !ip_mroute_opt(optname)) > - err = nf_setsockopt(sk, PF_INET, optname, optval, optlen); > + err = nf_setsockopt(sk, PF_INET, optname, USER_SOCKPTR(optval), > + optlen); > #endif > return err; > } > diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c > index 4697d09c98dc3e..f2a9680303d8c0 100644 > --- a/net/ipv4/netfilter/ip_tables.c > +++ b/net/ipv4/netfilter/ip_tables.c > @@ -1102,7 +1102,7 @@ __do_replace(struct net *net, const char *name, unsigned int valid_hooks, > } > > static int > -do_replace(struct net *net, const void __user *user, unsigned int len) > +do_replace(struct net *net, sockptr_t arg, unsigned int len) > { > int ret; > struct ipt_replace tmp; > @@ -1110,7 +1110,7 @@ do_replace(struct net *net, const void __user *user, unsigned int len) > void *loc_cpu_entry; > struct ipt_entry *iter; > > - if (copy_from_user(&tmp, user, sizeof(tmp)) != 0) > + if (copy_from_sockptr(&tmp, arg, sizeof(tmp)) != 0) > return -EFAULT; > > /* overflow check */ > @@ -1126,8 +1126,8 @@ do_replace(struct net *net, const void __user *user, unsigned int len) > return -ENOMEM; > > loc_cpu_entry = newinfo->entries; > - if (copy_from_user(loc_cpu_entry, user + sizeof(tmp), > - tmp.size) != 0) { > + sockptr_advance(arg, sizeof(tmp)); > + if (copy_from_sockptr(loc_cpu_entry, arg, tmp.size) != 0) { > ret = -EFAULT; > goto free_newinfo; > } Something along this path seems to have broken with this patch. An invocation of `iptables -A INPUT -m length --length 1360 -j DROP` now fails, with nf_setsockopt->do_replace->translate_table->check_entry_size_and_hooks: (unsigned char *)e + e->next_offset > limit ==> TRUE resulting in the whole call chain returning -EINVAL. It bisects back to this commit. This is on net-next. Jason
On Mon, Jul 27, 2020 at 05:03:10PM +0200, Jason A. Donenfeld wrote: > Hi Christoph, > > On Thu, Jul 23, 2020 at 08:08:54AM +0200, Christoph Hellwig wrote: > > diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c > > index da933f99b5d517..42befbf12846c0 100644 > > --- a/net/ipv4/ip_sockglue.c > > +++ b/net/ipv4/ip_sockglue.c > > @@ -1422,7 +1422,8 @@ int ip_setsockopt(struct sock *sk, int level, > > optname != IP_IPSEC_POLICY && > > optname != IP_XFRM_POLICY && > > !ip_mroute_opt(optname)) > > - err = nf_setsockopt(sk, PF_INET, optname, optval, optlen); > > + err = nf_setsockopt(sk, PF_INET, optname, USER_SOCKPTR(optval), > > + optlen); > > #endif > > return err; > > } > > diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c > > index 4697d09c98dc3e..f2a9680303d8c0 100644 > > --- a/net/ipv4/netfilter/ip_tables.c > > +++ b/net/ipv4/netfilter/ip_tables.c > > @@ -1102,7 +1102,7 @@ __do_replace(struct net *net, const char *name, unsigned int valid_hooks, > > } > > > > static int > > -do_replace(struct net *net, const void __user *user, unsigned int len) > > +do_replace(struct net *net, sockptr_t arg, unsigned int len) > > { > > int ret; > > struct ipt_replace tmp; > > @@ -1110,7 +1110,7 @@ do_replace(struct net *net, const void __user *user, unsigned int len) > > void *loc_cpu_entry; > > struct ipt_entry *iter; > > > > - if (copy_from_user(&tmp, user, sizeof(tmp)) != 0) > > + if (copy_from_sockptr(&tmp, arg, sizeof(tmp)) != 0) > > return -EFAULT; > > > > /* overflow check */ > > @@ -1126,8 +1126,8 @@ do_replace(struct net *net, const void __user *user, unsigned int len) > > return -ENOMEM; > > > > loc_cpu_entry = newinfo->entries; > > - if (copy_from_user(loc_cpu_entry, user + sizeof(tmp), > > - tmp.size) != 0) { > > + sockptr_advance(arg, sizeof(tmp)); > > + if (copy_from_sockptr(loc_cpu_entry, arg, tmp.size) != 0) { > > ret = -EFAULT; > > goto free_newinfo; > > } > > Something along this path seems to have broken with this patch. An > invocation of `iptables -A INPUT -m length --length 1360 -j DROP` now > fails, with > > nf_setsockopt->do_replace->translate_table->check_entry_size_and_hooks: > (unsigned char *)e + e->next_offset > limit ==> TRUE > > resulting in the whole call chain returning -EINVAL. It bisects back to > this commit. This is on net-next. This is another use o sockptr_advance that Ido already found a problem in. I'm looking into this at the moment..
Can you try the patch below? --- From cce2d2e1b43ecee5f4af7cf116808b74b330080f Mon Sep 17 00:00:00 2001 From: Christoph Hellwig <hch@lst.de> Date: Mon, 27 Jul 2020 17:42:27 +0200 Subject: net: remove sockptr_advance sockptr_advance never properly worked. Replace it with _offset variants of copy_from_sockptr and copy_to_sockptr. Fixes: ba423fdaa589 ("net: add a new sockptr_t type") Reported-by: Jason A. Donenfeld <Jason@zx2c4.com> Reported-by: Ido Schimmel <idosch@idosch.org> Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/crypto/chelsio/chtls/chtls_main.c | 12 +++++----- include/linux/sockptr.h | 27 +++++++++++------------ net/dccp/proto.c | 5 ++--- net/ipv4/netfilter/arp_tables.c | 8 +++---- net/ipv4/netfilter/ip_tables.c | 8 +++---- net/ipv4/tcp.c | 5 +++-- net/ipv6/ip6_flowlabel.c | 11 ++++----- net/ipv6/netfilter/ip6_tables.c | 8 +++---- net/netfilter/x_tables.c | 7 +++--- net/tls/tls_main.c | 6 ++--- 10 files changed, 49 insertions(+), 48 deletions(-) diff --git a/drivers/crypto/chelsio/chtls/chtls_main.c b/drivers/crypto/chelsio/chtls/chtls_main.c index c3058dcdb33c5c..66d247efd5615b 100644 --- a/drivers/crypto/chelsio/chtls/chtls_main.c +++ b/drivers/crypto/chelsio/chtls/chtls_main.c @@ -525,9 +525,9 @@ static int do_chtls_setsockopt(struct sock *sk, int optname, /* Obtain version and type from previous copy */ crypto_info[0] = tmp_crypto_info; /* Now copy the following data */ - sockptr_advance(optval, sizeof(*crypto_info)); - rc = copy_from_sockptr((char *)crypto_info + sizeof(*crypto_info), - optval, + rc = copy_from_sockptr_offset((char *)crypto_info + + sizeof(*crypto_info), + optval, sizeof(*crypto_info), sizeof(struct tls12_crypto_info_aes_gcm_128) - sizeof(*crypto_info)); @@ -542,9 +542,9 @@ static int do_chtls_setsockopt(struct sock *sk, int optname, } case TLS_CIPHER_AES_GCM_256: { crypto_info[0] = tmp_crypto_info; - sockptr_advance(optval, sizeof(*crypto_info)); - rc = copy_from_sockptr((char *)crypto_info + sizeof(*crypto_info), - optval, + rc = copy_from_sockptr_offset((char *)crypto_info + + sizeof(*crypto_info), + optval, sizeof(*crypto_info), sizeof(struct tls12_crypto_info_aes_gcm_256) - sizeof(*crypto_info)); diff --git a/include/linux/sockptr.h b/include/linux/sockptr.h index b13ea1422f93a5..9e6c81d474cba8 100644 --- a/include/linux/sockptr.h +++ b/include/linux/sockptr.h @@ -69,19 +69,26 @@ static inline bool sockptr_is_null(sockptr_t sockptr) return !sockptr.user; } -static inline int copy_from_sockptr(void *dst, sockptr_t src, size_t size) +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, size); - memcpy(dst, src.kernel, size); + return copy_from_user(dst, src.user + offset, size); + memcpy(dst, src.kernel + offset, size); return 0; } -static inline int copy_to_sockptr(sockptr_t dst, const void *src, size_t size) +static inline int copy_from_sockptr(void *dst, sockptr_t src, size_t size) +{ + return copy_from_sockptr_offset(dst, src, 0, size); +} + +static inline int copy_to_sockptr_offset(sockptr_t dst, size_t offset, + const void *src, size_t size) { if (!sockptr_is_kernel(dst)) - return copy_to_user(dst.user, src, size); - memcpy(dst.kernel, src, size); + return copy_to_user(dst.user + offset, src, size); + memcpy(dst.kernel + offset, src, size); return 0; } @@ -112,14 +119,6 @@ static inline void *memdup_sockptr_nul(sockptr_t src, size_t len) return p; } -static inline void sockptr_advance(sockptr_t sockptr, size_t len) -{ - if (sockptr_is_kernel(sockptr)) - sockptr.kernel += len; - else - sockptr.user += len; -} - static inline long strncpy_from_sockptr(char *dst, sockptr_t src, size_t count) { if (sockptr_is_kernel(src)) { diff --git a/net/dccp/proto.c b/net/dccp/proto.c index 2e9e8449698fb4..d148ab1530e57b 100644 --- a/net/dccp/proto.c +++ b/net/dccp/proto.c @@ -426,9 +426,8 @@ static int dccp_setsockopt_service(struct sock *sk, const __be32 service, return -ENOMEM; sl->dccpsl_nr = optlen / sizeof(u32) - 1; - sockptr_advance(optval, sizeof(service)); - if (copy_from_sockptr(sl->dccpsl_list, optval, - optlen - sizeof(service)) || + if (copy_from_sockptr_offset(sl->dccpsl_list, optval, + sizeof(service), optlen - sizeof(service)) || dccp_list_has_service(sl, DCCP_SERVICE_INVALID_VALUE)) { kfree(sl); return -EFAULT; diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c index 9a1567dbc022b6..d1e04d2b5170ec 100644 --- a/net/ipv4/netfilter/arp_tables.c +++ b/net/ipv4/netfilter/arp_tables.c @@ -971,8 +971,8 @@ static int do_replace(struct net *net, sockptr_t arg, unsigned int len) return -ENOMEM; loc_cpu_entry = newinfo->entries; - sockptr_advance(arg, sizeof(tmp)); - if (copy_from_sockptr(loc_cpu_entry, arg, tmp.size) != 0) { + if (copy_from_sockptr_offset(loc_cpu_entry, arg, sizeof(tmp), + tmp.size) != 0) { ret = -EFAULT; goto free_newinfo; } @@ -1267,8 +1267,8 @@ static int compat_do_replace(struct net *net, sockptr_t arg, unsigned int len) return -ENOMEM; loc_cpu_entry = newinfo->entries; - sockptr_advance(arg, sizeof(tmp)); - if (copy_from_sockptr(loc_cpu_entry, arg, tmp.size) != 0) { + if (copy_from_sockptr_offset(loc_cpu_entry, arg, sizeof(tmp), + tmp.size) != 0) { ret = -EFAULT; goto free_newinfo; } diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c index f2a9680303d8c0..f15bc21d730164 100644 --- a/net/ipv4/netfilter/ip_tables.c +++ b/net/ipv4/netfilter/ip_tables.c @@ -1126,8 +1126,8 @@ do_replace(struct net *net, sockptr_t arg, unsigned int len) return -ENOMEM; loc_cpu_entry = newinfo->entries; - sockptr_advance(arg, sizeof(tmp)); - if (copy_from_sockptr(loc_cpu_entry, arg, tmp.size) != 0) { + if (copy_from_sockptr_offset(loc_cpu_entry, arg, sizeof(tmp), + tmp.size) != 0) { ret = -EFAULT; goto free_newinfo; } @@ -1508,8 +1508,8 @@ compat_do_replace(struct net *net, sockptr_t arg, unsigned int len) return -ENOMEM; loc_cpu_entry = newinfo->entries; - sockptr_advance(arg, sizeof(tmp)); - if (copy_from_sockptr(loc_cpu_entry, arg, tmp.size) != 0) { + if (copy_from_sockptr_offset(loc_cpu_entry, arg, sizeof(tmp), + tmp.size) != 0) { ret = -EFAULT; goto free_newinfo; } diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index 27de9380ed140e..4afec552f211b9 100644 --- a/net/ipv4/tcp.c +++ b/net/ipv4/tcp.c @@ -2801,12 +2801,13 @@ static int tcp_repair_options_est(struct sock *sk, sockptr_t optbuf, { struct tcp_sock *tp = tcp_sk(sk); struct tcp_repair_opt opt; + size_t offset = 0; while (len >= sizeof(opt)) { - if (copy_from_sockptr(&opt, optbuf, sizeof(opt))) + if (copy_from_sockptr_offset(&opt, optbuf, offset, sizeof(opt))) return -EFAULT; - sockptr_advance(optbuf, sizeof(opt)); + offset += sizeof(opt); len -= sizeof(opt); switch (opt.opt_code) { diff --git a/net/ipv6/ip6_flowlabel.c b/net/ipv6/ip6_flowlabel.c index 215b6f5e733ec9..2d655260dedc75 100644 --- a/net/ipv6/ip6_flowlabel.c +++ b/net/ipv6/ip6_flowlabel.c @@ -401,8 +401,8 @@ fl_create(struct net *net, struct sock *sk, struct in6_flowlabel_req *freq, memset(fl->opt, 0, sizeof(*fl->opt)); fl->opt->tot_len = sizeof(*fl->opt) + olen; err = -EFAULT; - sockptr_advance(optval, CMSG_ALIGN(sizeof(*freq))); - if (copy_from_sockptr(fl->opt + 1, optval, olen)) + if (copy_from_sockptr_offset(fl->opt + 1, optval, + CMSG_ALIGN(sizeof(*freq)), olen)) goto done; msg.msg_controllen = olen; @@ -703,9 +703,10 @@ static int ipv6_flowlabel_get(struct sock *sk, struct in6_flowlabel_req *freq, goto recheck; if (!freq->flr_label) { - sockptr_advance(optval, - offsetof(struct in6_flowlabel_req, flr_label)); - if (copy_to_sockptr(optval, &fl->label, sizeof(fl->label))) { + size_t offset = offsetof(struct in6_flowlabel_req, flr_label); + + if (copy_to_sockptr_offset(optval, offset, &fl->label, + sizeof(fl->label))) { /* Intentionally ignore fault. */ } } diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c index 1d52957a413f4a..2e2119bfcf1373 100644 --- a/net/ipv6/netfilter/ip6_tables.c +++ b/net/ipv6/netfilter/ip6_tables.c @@ -1143,8 +1143,8 @@ do_replace(struct net *net, sockptr_t arg, unsigned int len) return -ENOMEM; loc_cpu_entry = newinfo->entries; - sockptr_advance(arg, sizeof(tmp)); - if (copy_from_sockptr(loc_cpu_entry, arg, tmp.size) != 0) { + if (copy_from_sockptr_offset(loc_cpu_entry, arg, sizeof(tmp), + tmp.size) != 0) { ret = -EFAULT; goto free_newinfo; } @@ -1517,8 +1517,8 @@ compat_do_replace(struct net *net, sockptr_t arg, unsigned int len) return -ENOMEM; loc_cpu_entry = newinfo->entries; - sockptr_advance(arg, sizeof(tmp)); - if (copy_from_sockptr(loc_cpu_entry, arg, tmp.size) != 0) { + if (copy_from_sockptr_offset(loc_cpu_entry, arg, sizeof(tmp), + tmp.size) != 0) { ret = -EFAULT; goto free_newinfo; } diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c index b97eb4b538fd4e..91bf6635ea9ee4 100644 --- a/net/netfilter/x_tables.c +++ b/net/netfilter/x_tables.c @@ -1050,6 +1050,7 @@ EXPORT_SYMBOL_GPL(xt_check_target); void *xt_copy_counters(sockptr_t arg, unsigned int len, struct xt_counters_info *info) { + size_t offset; void *mem; u64 size; @@ -1067,7 +1068,7 @@ void *xt_copy_counters(sockptr_t arg, unsigned int len, memcpy(info->name, compat_tmp.name, sizeof(info->name) - 1); info->num_counters = compat_tmp.num_counters; - sockptr_advance(arg, sizeof(compat_tmp)); + offset = sizeof(compat_tmp); } else #endif { @@ -1078,7 +1079,7 @@ void *xt_copy_counters(sockptr_t arg, unsigned int len, if (copy_from_sockptr(info, arg, sizeof(*info)) != 0) return ERR_PTR(-EFAULT); - sockptr_advance(arg, sizeof(*info)); + offset = sizeof(*info); } info->name[sizeof(info->name) - 1] = '\0'; @@ -1092,7 +1093,7 @@ void *xt_copy_counters(sockptr_t arg, unsigned int len, if (!mem) return ERR_PTR(-ENOMEM); - if (copy_from_sockptr(mem, arg, len) == 0) + if (copy_from_sockptr_offset(mem, arg, offset, len) == 0) return mem; vfree(mem); diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c index d77f7d821130db..bbc52b088d2968 100644 --- a/net/tls/tls_main.c +++ b/net/tls/tls_main.c @@ -522,9 +522,9 @@ static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval, goto err_crypto_info; } - sockptr_advance(optval, sizeof(*crypto_info)); - rc = copy_from_sockptr(crypto_info + 1, optval, - optlen - sizeof(*crypto_info)); + rc = copy_from_sockptr_offset(crypto_info + 1, optval, + sizeof(*crypto_info), + optlen - sizeof(*crypto_info)); if (rc) { rc = -EFAULT; goto err_crypto_info;
On Mon, Jul 27, 2020 at 5:06 PM Christoph Hellwig <hch@lst.de> wrote: > > On Mon, Jul 27, 2020 at 05:03:10PM +0200, Jason A. Donenfeld wrote: > > Hi Christoph, > > > > On Thu, Jul 23, 2020 at 08:08:54AM +0200, Christoph Hellwig wrote: > > > diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c > > > index da933f99b5d517..42befbf12846c0 100644 > > > --- a/net/ipv4/ip_sockglue.c > > > +++ b/net/ipv4/ip_sockglue.c > > > @@ -1422,7 +1422,8 @@ int ip_setsockopt(struct sock *sk, int level, > > > optname != IP_IPSEC_POLICY && > > > optname != IP_XFRM_POLICY && > > > !ip_mroute_opt(optname)) > > > - err = nf_setsockopt(sk, PF_INET, optname, optval, optlen); > > > + err = nf_setsockopt(sk, PF_INET, optname, USER_SOCKPTR(optval), > > > + optlen); > > > #endif > > > return err; > > > } > > > diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c > > > index 4697d09c98dc3e..f2a9680303d8c0 100644 > > > --- a/net/ipv4/netfilter/ip_tables.c > > > +++ b/net/ipv4/netfilter/ip_tables.c > > > @@ -1102,7 +1102,7 @@ __do_replace(struct net *net, const char *name, unsigned int valid_hooks, > > > } > > > > > > static int > > > -do_replace(struct net *net, const void __user *user, unsigned int len) > > > +do_replace(struct net *net, sockptr_t arg, unsigned int len) > > > { > > > int ret; > > > struct ipt_replace tmp; > > > @@ -1110,7 +1110,7 @@ do_replace(struct net *net, const void __user *user, unsigned int len) > > > void *loc_cpu_entry; > > > struct ipt_entry *iter; > > > > > > - if (copy_from_user(&tmp, user, sizeof(tmp)) != 0) > > > + if (copy_from_sockptr(&tmp, arg, sizeof(tmp)) != 0) > > > return -EFAULT; > > > > > > /* overflow check */ > > > @@ -1126,8 +1126,8 @@ do_replace(struct net *net, const void __user *user, unsigned int len) > > > return -ENOMEM; > > > > > > loc_cpu_entry = newinfo->entries; > > > - if (copy_from_user(loc_cpu_entry, user + sizeof(tmp), > > > - tmp.size) != 0) { > > > + sockptr_advance(arg, sizeof(tmp)); > > > + if (copy_from_sockptr(loc_cpu_entry, arg, tmp.size) != 0) { > > > ret = -EFAULT; > > > goto free_newinfo; > > > } > > > > Something along this path seems to have broken with this patch. An > > invocation of `iptables -A INPUT -m length --length 1360 -j DROP` now > > fails, with > > > > nf_setsockopt->do_replace->translate_table->check_entry_size_and_hooks: > > (unsigned char *)e + e->next_offset > limit ==> TRUE > > > > resulting in the whole call chain returning -EINVAL. It bisects back to > > this commit. This is on net-next. > > This is another use o sockptr_advance that Ido already found a problem > in. I'm looking into this at the moment.. I haven't seen Ido's patch, but it seems clear the issue is that you want to call `sockptr_advance(&arg, sizeof(tmp))`, and adjust sockptr_advance to take a pointer. Slight concern about the whole concept: Things are defined as typedef union { void *kernel; void __user *user; } sockptr_t; static inline bool sockptr_is_kernel(sockptr_t sockptr) { return (unsigned long)sockptr.kernel >= TASK_SIZE; } So what happens if we have some code like: sockptr_t sp; init_user_sockptr(&sp, user_controlled_struct.extra_user_ptr); sockptr_advance(&sp, user_controlled_struct.some_big_offset); copy_to_sockptr(&sp, user_controlled_struct.a_few_bytes, sizeof(user_controlled_struct.a_few_bytes)); With the user controlling some_big_offset, he can convert the user sockptr into a kernel sockptr, causing the subsequent copy_to_sockptr to be a vanilla memcpy, after which a security disaster ensues. Maybe sockptr_advance should have some safety checks and sometimes return -EFAULT? Or you should always use the implementation where being a kernel address is an explicit bit of sockptr_t, rather than being implicit?
> From cce2d2e1b43ecee5f4af7cf116808b74b330080f Mon Sep 17 00:00:00 2001 > From: Christoph Hellwig <hch@lst.de> > Date: Mon, 27 Jul 2020 17:42:27 +0200 > Subject: net: remove sockptr_advance > > sockptr_advance never properly worked. Replace it with _offset variants > of copy_from_sockptr and copy_to_sockptr. > > Fixes: ba423fdaa589 ("net: add a new sockptr_t type") > Reported-by: Jason A. Donenfeld <Jason@zx2c4.com> > Reported-by: Ido Schimmel <idosch@idosch.org> > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > drivers/crypto/chelsio/chtls/chtls_main.c | 12 +++++----- > include/linux/sockptr.h | 27 +++++++++++------------ > net/dccp/proto.c | 5 ++--- > net/ipv4/netfilter/arp_tables.c | 8 +++---- > net/ipv4/netfilter/ip_tables.c | 8 +++---- > net/ipv4/tcp.c | 5 +++-- > net/ipv6/ip6_flowlabel.c | 11 ++++----- > net/ipv6/netfilter/ip6_tables.c | 8 +++---- > net/netfilter/x_tables.c | 7 +++--- > net/tls/tls_main.c | 6 ++--- > 10 files changed, 49 insertions(+), 48 deletions(-) > > diff --git a/drivers/crypto/chelsio/chtls/chtls_main.c b/drivers/crypto/chelsio/chtls/chtls_main.c > index c3058dcdb33c5c..66d247efd5615b 100644 > --- a/drivers/crypto/chelsio/chtls/chtls_main.c > +++ b/drivers/crypto/chelsio/chtls/chtls_main.c > @@ -525,9 +525,9 @@ static int do_chtls_setsockopt(struct sock *sk, int optname, > /* Obtain version and type from previous copy */ > crypto_info[0] = tmp_crypto_info; > /* Now copy the following data */ > - sockptr_advance(optval, sizeof(*crypto_info)); > - rc = copy_from_sockptr((char *)crypto_info + sizeof(*crypto_info), > - optval, > + rc = copy_from_sockptr_offset((char *)crypto_info + > + sizeof(*crypto_info), > + optval, sizeof(*crypto_info), > sizeof(struct tls12_crypto_info_aes_gcm_128) > - sizeof(*crypto_info)); > > @@ -542,9 +542,9 @@ static int do_chtls_setsockopt(struct sock *sk, int optname, > } > case TLS_CIPHER_AES_GCM_256: { > crypto_info[0] = tmp_crypto_info; > - sockptr_advance(optval, sizeof(*crypto_info)); > - rc = copy_from_sockptr((char *)crypto_info + sizeof(*crypto_info), > - optval, > + rc = copy_from_sockptr_offset((char *)crypto_info + > + sizeof(*crypto_info), > + optval, sizeof(*crypto_info), > sizeof(struct tls12_crypto_info_aes_gcm_256) > - sizeof(*crypto_info)); > > diff --git a/include/linux/sockptr.h b/include/linux/sockptr.h > index b13ea1422f93a5..9e6c81d474cba8 100644 > --- a/include/linux/sockptr.h > +++ b/include/linux/sockptr.h > @@ -69,19 +69,26 @@ static inline bool sockptr_is_null(sockptr_t sockptr) > return !sockptr.user; > } > > -static inline int copy_from_sockptr(void *dst, sockptr_t src, size_t size) > +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, size); > - memcpy(dst, src.kernel, size); > + return copy_from_user(dst, src.user + offset, size); > + memcpy(dst, src.kernel + offset, size); > return 0; > } > > -static inline int copy_to_sockptr(sockptr_t dst, const void *src, size_t size) > +static inline int copy_from_sockptr(void *dst, sockptr_t src, size_t size) > +{ > + return copy_from_sockptr_offset(dst, src, 0, size); > +} > + > +static inline int copy_to_sockptr_offset(sockptr_t dst, size_t offset, > + const void *src, size_t size) > { > if (!sockptr_is_kernel(dst)) > - return copy_to_user(dst.user, src, size); > - memcpy(dst.kernel, src, size); > + return copy_to_user(dst.user + offset, src, size); > + memcpy(dst.kernel + offset, src, size); > return 0; > } > > @@ -112,14 +119,6 @@ static inline void *memdup_sockptr_nul(sockptr_t src, size_t len) > return p; > } > > -static inline void sockptr_advance(sockptr_t sockptr, size_t len) > -{ > - if (sockptr_is_kernel(sockptr)) > - sockptr.kernel += len; > - else > - sockptr.user += len; > -} > - > static inline long strncpy_from_sockptr(char *dst, sockptr_t src, size_t count) > { > if (sockptr_is_kernel(src)) { > diff --git a/net/dccp/proto.c b/net/dccp/proto.c > index 2e9e8449698fb4..d148ab1530e57b 100644 > --- a/net/dccp/proto.c > +++ b/net/dccp/proto.c > @@ -426,9 +426,8 @@ static int dccp_setsockopt_service(struct sock *sk, const __be32 service, > return -ENOMEM; > > sl->dccpsl_nr = optlen / sizeof(u32) - 1; > - sockptr_advance(optval, sizeof(service)); > - if (copy_from_sockptr(sl->dccpsl_list, optval, > - optlen - sizeof(service)) || > + if (copy_from_sockptr_offset(sl->dccpsl_list, optval, > + sizeof(service), optlen - sizeof(service)) || > dccp_list_has_service(sl, DCCP_SERVICE_INVALID_VALUE)) { > kfree(sl); > return -EFAULT; > diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c > index 9a1567dbc022b6..d1e04d2b5170ec 100644 > --- a/net/ipv4/netfilter/arp_tables.c > +++ b/net/ipv4/netfilter/arp_tables.c > @@ -971,8 +971,8 @@ static int do_replace(struct net *net, sockptr_t arg, unsigned int len) > return -ENOMEM; > > loc_cpu_entry = newinfo->entries; > - sockptr_advance(arg, sizeof(tmp)); > - if (copy_from_sockptr(loc_cpu_entry, arg, tmp.size) != 0) { > + if (copy_from_sockptr_offset(loc_cpu_entry, arg, sizeof(tmp), > + tmp.size) != 0) { > ret = -EFAULT; > goto free_newinfo; > } > @@ -1267,8 +1267,8 @@ static int compat_do_replace(struct net *net, sockptr_t arg, unsigned int len) > return -ENOMEM; > > loc_cpu_entry = newinfo->entries; > - sockptr_advance(arg, sizeof(tmp)); > - if (copy_from_sockptr(loc_cpu_entry, arg, tmp.size) != 0) { > + if (copy_from_sockptr_offset(loc_cpu_entry, arg, sizeof(tmp), > + tmp.size) != 0) { > ret = -EFAULT; > goto free_newinfo; > } > diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c > index f2a9680303d8c0..f15bc21d730164 100644 > --- a/net/ipv4/netfilter/ip_tables.c > +++ b/net/ipv4/netfilter/ip_tables.c > @@ -1126,8 +1126,8 @@ do_replace(struct net *net, sockptr_t arg, unsigned int len) > return -ENOMEM; > > loc_cpu_entry = newinfo->entries; > - sockptr_advance(arg, sizeof(tmp)); > - if (copy_from_sockptr(loc_cpu_entry, arg, tmp.size) != 0) { > + if (copy_from_sockptr_offset(loc_cpu_entry, arg, sizeof(tmp), > + tmp.size) != 0) { > ret = -EFAULT; > goto free_newinfo; > } > @@ -1508,8 +1508,8 @@ compat_do_replace(struct net *net, sockptr_t arg, unsigned int len) > return -ENOMEM; > > loc_cpu_entry = newinfo->entries; > - sockptr_advance(arg, sizeof(tmp)); > - if (copy_from_sockptr(loc_cpu_entry, arg, tmp.size) != 0) { > + if (copy_from_sockptr_offset(loc_cpu_entry, arg, sizeof(tmp), > + tmp.size) != 0) { > ret = -EFAULT; > goto free_newinfo; > } > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c > index 27de9380ed140e..4afec552f211b9 100644 > --- a/net/ipv4/tcp.c > +++ b/net/ipv4/tcp.c > @@ -2801,12 +2801,13 @@ static int tcp_repair_options_est(struct sock *sk, sockptr_t optbuf, > { > struct tcp_sock *tp = tcp_sk(sk); > struct tcp_repair_opt opt; > + size_t offset = 0; > > while (len >= sizeof(opt)) { > - if (copy_from_sockptr(&opt, optbuf, sizeof(opt))) > + if (copy_from_sockptr_offset(&opt, optbuf, offset, sizeof(opt))) > return -EFAULT; > > - sockptr_advance(optbuf, sizeof(opt)); > + offset += sizeof(opt); > len -= sizeof(opt); > > switch (opt.opt_code) { > diff --git a/net/ipv6/ip6_flowlabel.c b/net/ipv6/ip6_flowlabel.c > index 215b6f5e733ec9..2d655260dedc75 100644 > --- a/net/ipv6/ip6_flowlabel.c > +++ b/net/ipv6/ip6_flowlabel.c > @@ -401,8 +401,8 @@ fl_create(struct net *net, struct sock *sk, struct in6_flowlabel_req *freq, > memset(fl->opt, 0, sizeof(*fl->opt)); > fl->opt->tot_len = sizeof(*fl->opt) + olen; > err = -EFAULT; > - sockptr_advance(optval, CMSG_ALIGN(sizeof(*freq))); > - if (copy_from_sockptr(fl->opt + 1, optval, olen)) > + if (copy_from_sockptr_offset(fl->opt + 1, optval, > + CMSG_ALIGN(sizeof(*freq)), olen)) > goto done; > > msg.msg_controllen = olen; > @@ -703,9 +703,10 @@ static int ipv6_flowlabel_get(struct sock *sk, struct in6_flowlabel_req *freq, > goto recheck; > > if (!freq->flr_label) { > - sockptr_advance(optval, > - offsetof(struct in6_flowlabel_req, flr_label)); > - if (copy_to_sockptr(optval, &fl->label, sizeof(fl->label))) { > + size_t offset = offsetof(struct in6_flowlabel_req, flr_label); > + > + if (copy_to_sockptr_offset(optval, offset, &fl->label, > + sizeof(fl->label))) { > /* Intentionally ignore fault. */ > } > } > diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c > index 1d52957a413f4a..2e2119bfcf1373 100644 > --- a/net/ipv6/netfilter/ip6_tables.c > +++ b/net/ipv6/netfilter/ip6_tables.c > @@ -1143,8 +1143,8 @@ do_replace(struct net *net, sockptr_t arg, unsigned int len) > return -ENOMEM; > > loc_cpu_entry = newinfo->entries; > - sockptr_advance(arg, sizeof(tmp)); > - if (copy_from_sockptr(loc_cpu_entry, arg, tmp.size) != 0) { > + if (copy_from_sockptr_offset(loc_cpu_entry, arg, sizeof(tmp), > + tmp.size) != 0) { > ret = -EFAULT; > goto free_newinfo; > } > @@ -1517,8 +1517,8 @@ compat_do_replace(struct net *net, sockptr_t arg, unsigned int len) > return -ENOMEM; > > loc_cpu_entry = newinfo->entries; > - sockptr_advance(arg, sizeof(tmp)); > - if (copy_from_sockptr(loc_cpu_entry, arg, tmp.size) != 0) { > + if (copy_from_sockptr_offset(loc_cpu_entry, arg, sizeof(tmp), > + tmp.size) != 0) { > ret = -EFAULT; > goto free_newinfo; > } > diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c > index b97eb4b538fd4e..91bf6635ea9ee4 100644 > --- a/net/netfilter/x_tables.c > +++ b/net/netfilter/x_tables.c > @@ -1050,6 +1050,7 @@ EXPORT_SYMBOL_GPL(xt_check_target); > void *xt_copy_counters(sockptr_t arg, unsigned int len, > struct xt_counters_info *info) > { > + size_t offset; > void *mem; > u64 size; > > @@ -1067,7 +1068,7 @@ void *xt_copy_counters(sockptr_t arg, unsigned int len, > > memcpy(info->name, compat_tmp.name, sizeof(info->name) - 1); > info->num_counters = compat_tmp.num_counters; > - sockptr_advance(arg, sizeof(compat_tmp)); > + offset = sizeof(compat_tmp); > } else > #endif > { > @@ -1078,7 +1079,7 @@ void *xt_copy_counters(sockptr_t arg, unsigned int len, > if (copy_from_sockptr(info, arg, sizeof(*info)) != 0) > return ERR_PTR(-EFAULT); > > - sockptr_advance(arg, sizeof(*info)); > + offset = sizeof(*info); > } > info->name[sizeof(info->name) - 1] = '\0'; > > @@ -1092,7 +1093,7 @@ void *xt_copy_counters(sockptr_t arg, unsigned int len, > if (!mem) > return ERR_PTR(-ENOMEM); > > - if (copy_from_sockptr(mem, arg, len) == 0) > + if (copy_from_sockptr_offset(mem, arg, offset, len) == 0) > return mem; > > vfree(mem); > diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c > index d77f7d821130db..bbc52b088d2968 100644 > --- a/net/tls/tls_main.c > +++ b/net/tls/tls_main.c > @@ -522,9 +522,9 @@ static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval, > goto err_crypto_info; > } > > - sockptr_advance(optval, sizeof(*crypto_info)); > - rc = copy_from_sockptr(crypto_info + 1, optval, > - optlen - sizeof(*crypto_info)); > + rc = copy_from_sockptr_offset(crypto_info + 1, optval, > + sizeof(*crypto_info), > + optlen - sizeof(*crypto_info)); > if (rc) { > rc = -EFAULT; > goto err_crypto_info; > -- > 2.27.0 Getting rid of sockptr_advance entirely seems like the right decision here. You still might want to make sure the addition in copy_from_sockptr_offset doesn't overflow, and return -EFAULT if it does. But this indeed fixes the bug, so: Acked-by: Jason A. Donenfeld <Jason@zx2c4.com>
On Mon, Jul 27, 2020 at 06:16:32PM +0200, Jason A. Donenfeld wrote: > Maybe sockptr_advance should have some safety checks and sometimes > return -EFAULT? Or you should always use the implementation where > being a kernel address is an explicit bit of sockptr_t, rather than > being implicit? I already have a patch to use access_ok to check the whole range in init_user_sockptr.
From: Christoph Hellwig > Sent: 27 July 2020 17:24 > > On Mon, Jul 27, 2020 at 06:16:32PM +0200, Jason A. Donenfeld wrote: > > Maybe sockptr_advance should have some safety checks and sometimes > > return -EFAULT? Or you should always use the implementation where > > being a kernel address is an explicit bit of sockptr_t, rather than > > being implicit? > > I already have a patch to use access_ok to check the whole range in > init_user_sockptr. That doesn't make (much) difference to the code paths that ignore the user-supplied length. OTOH doing the user/kernel check on the base address (not an incremented one) means that the correct copy function is always selected. Perhaps the functions should all be passed a 'const sockptr_t'. The typedef could be made 'const' - requiring non-const items explicitly use the union/struct itself. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Tue, Jul 28, 2020 at 10:07 AM David Laight <David.Laight@aculab.com> wrote: > > From: Christoph Hellwig > > Sent: 27 July 2020 17:24 > > > > On Mon, Jul 27, 2020 at 06:16:32PM +0200, Jason A. Donenfeld wrote: > > > Maybe sockptr_advance should have some safety checks and sometimes > > > return -EFAULT? Or you should always use the implementation where > > > being a kernel address is an explicit bit of sockptr_t, rather than > > > being implicit? > > > > I already have a patch to use access_ok to check the whole range in > > init_user_sockptr. > > That doesn't make (much) difference to the code paths that ignore > the user-supplied length. > OTOH doing the user/kernel check on the base address (not an > incremented one) means that the correct copy function is always > selected. Right, I had the same reaction in reading this, but actually, his code gets rid of the sockptr_advance stuff entirely and never mutates, so even though my point about attacking those pointers was missed, the code does the better thing now -- checking the base address and never mutating the pointer. So I think we're good. > > Perhaps the functions should all be passed a 'const sockptr_t'. > The typedef could be made 'const' - requiring non-const items > explicitly use the union/struct itself. I was thinking the same, but just by making the pointers inside the struct const. However, making the whole struct const via the typedef is a much better idea. That'd probably require changing the signature of init_user_sockptr a bit, which would be fine, but indeed I think this would be a very positive change. Jason
diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h index 711b4d4486f042..0101747de54936 100644 --- a/include/linux/netfilter.h +++ b/include/linux/netfilter.h @@ -13,6 +13,7 @@ #include <linux/static_key.h> #include <linux/netfilter_defs.h> #include <linux/netdevice.h> +#include <linux/sockptr.h> #include <net/net_namespace.h> static inline int NF_DROP_GETERR(int verdict) @@ -163,7 +164,8 @@ struct nf_sockopt_ops { /* Non-inclusive ranges: use 0/0/NULL to never get called. */ int set_optmin; int set_optmax; - int (*set)(struct sock *sk, int optval, void __user *user, unsigned int len); + int (*set)(struct sock *sk, int optval, sockptr_t arg, + unsigned int len); int get_optmin; int get_optmax; int (*get)(struct sock *sk, int optval, void __user *user, int *len); @@ -338,7 +340,7 @@ NF_HOOK_LIST(uint8_t pf, unsigned int hook, struct net *net, struct sock *sk, } /* Call setsockopt() */ -int nf_setsockopt(struct sock *sk, u_int8_t pf, int optval, char __user *opt, +int nf_setsockopt(struct sock *sk, u_int8_t pf, int optval, sockptr_t opt, unsigned int len); int nf_getsockopt(struct sock *sk, u_int8_t pf, int optval, char __user *opt, int *len); diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c index 12f8929667bf43..d35173e803d3fe 100644 --- a/net/bridge/netfilter/ebtables.c +++ b/net/bridge/netfilter/ebtables.c @@ -1063,14 +1063,13 @@ static int do_replace_finish(struct net *net, struct ebt_replace *repl, } /* replace the table */ -static int do_replace(struct net *net, const void __user *user, - unsigned int len) +static int do_replace(struct net *net, sockptr_t arg, unsigned int len) { int ret, countersize; struct ebt_table_info *newinfo; struct ebt_replace tmp; - if (copy_from_user(&tmp, user, sizeof(tmp)) != 0) + if (copy_from_sockptr(&tmp, arg, sizeof(tmp)) != 0) return -EFAULT; if (len != sizeof(tmp) + tmp.entries_size) @@ -1286,12 +1285,11 @@ static int do_update_counters(struct net *net, const char *name, return ret; } -static int update_counters(struct net *net, const void __user *user, - unsigned int len) +static int update_counters(struct net *net, sockptr_t arg, unsigned int len) { struct ebt_replace hlp; - if (copy_from_user(&hlp, user, sizeof(hlp))) + if (copy_from_sockptr(&hlp, arg, sizeof(hlp))) return -EFAULT; if (len != sizeof(hlp) + hlp.num_counters * sizeof(struct ebt_counter)) @@ -2079,7 +2077,7 @@ static int compat_copy_entries(unsigned char *data, unsigned int size_user, static int compat_copy_ebt_replace_from_user(struct ebt_replace *repl, - void __user *user, unsigned int len) + sockptr_t arg, unsigned int len) { struct compat_ebt_replace tmp; int i; @@ -2087,7 +2085,7 @@ static int compat_copy_ebt_replace_from_user(struct ebt_replace *repl, if (len < sizeof(tmp)) return -EINVAL; - if (copy_from_user(&tmp, user, sizeof(tmp))) + if (copy_from_sockptr(&tmp, arg, sizeof(tmp))) return -EFAULT; if (len != sizeof(tmp) + tmp.entries_size) @@ -2114,8 +2112,7 @@ static int compat_copy_ebt_replace_from_user(struct ebt_replace *repl, return 0; } -static int compat_do_replace(struct net *net, void __user *user, - unsigned int len) +static int compat_do_replace(struct net *net, sockptr_t arg, unsigned int len) { int ret, i, countersize, size64; struct ebt_table_info *newinfo; @@ -2123,10 +2120,10 @@ static int compat_do_replace(struct net *net, void __user *user, struct ebt_entries_buf_state state; void *entries_tmp; - ret = compat_copy_ebt_replace_from_user(&tmp, user, len); + ret = compat_copy_ebt_replace_from_user(&tmp, arg, len); if (ret) { /* try real handler in case userland supplied needed padding */ - if (ret == -EINVAL && do_replace(net, user, len) == 0) + if (ret == -EINVAL && do_replace(net, arg, len) == 0) ret = 0; return ret; } @@ -2217,17 +2214,17 @@ static int compat_do_replace(struct net *net, void __user *user, goto free_entries; } -static int compat_update_counters(struct net *net, void __user *user, +static int compat_update_counters(struct net *net, sockptr_t arg, unsigned int len) { struct compat_ebt_replace hlp; - if (copy_from_user(&hlp, user, sizeof(hlp))) + if (copy_from_sockptr(&hlp, arg, sizeof(hlp))) return -EFAULT; /* try real handler in case userland supplied needed padding */ if (len != sizeof(hlp) + hlp.num_counters * sizeof(struct ebt_counter)) - return update_counters(net, user, len); + return update_counters(net, arg, len); return do_update_counters(net, hlp.name, compat_ptr(hlp.counters), hlp.num_counters, len); @@ -2368,7 +2365,7 @@ static int do_ebt_get_ctl(struct sock *sk, int cmd, void __user *user, int *len) return ret; } -static int do_ebt_set_ctl(struct sock *sk, int cmd, void __user *user, +static int do_ebt_set_ctl(struct sock *sk, int cmd, sockptr_t arg, unsigned int len) { struct net *net = sock_net(sk); @@ -2381,18 +2378,18 @@ static int do_ebt_set_ctl(struct sock *sk, int cmd, void __user *user, case EBT_SO_SET_ENTRIES: #ifdef CONFIG_COMPAT if (in_compat_syscall()) - ret = compat_do_replace(net, user, len); + ret = compat_do_replace(net, arg, len); else #endif - ret = do_replace(net, user, len); + ret = do_replace(net, arg, len); break; case EBT_SO_SET_COUNTERS: #ifdef CONFIG_COMPAT if (in_compat_syscall()) - ret = compat_update_counters(net, user, len); + ret = compat_update_counters(net, arg, len); else #endif - ret = update_counters(net, user, len); + ret = update_counters(net, arg, len); break; default: ret = -EINVAL; diff --git a/net/decnet/af_decnet.c b/net/decnet/af_decnet.c index 7d7ae2dd69b8ad..7d51ab608fb3f1 100644 --- a/net/decnet/af_decnet.c +++ b/net/decnet/af_decnet.c @@ -1332,7 +1332,8 @@ static int dn_setsockopt(struct socket *sock, int level, int optname, char __use /* we need to exclude all possible ENOPROTOOPTs except default case */ if (err == -ENOPROTOOPT && optname != DSO_LINKINFO && optname != DSO_STREAM && optname != DSO_SEQPACKET) - err = nf_setsockopt(sk, PF_DECnet, optname, optval, optlen); + err = nf_setsockopt(sk, PF_DECnet, optname, + USER_SOCKPTR(optval), optlen); #endif return err; diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c index da933f99b5d517..42befbf12846c0 100644 --- a/net/ipv4/ip_sockglue.c +++ b/net/ipv4/ip_sockglue.c @@ -1422,7 +1422,8 @@ int ip_setsockopt(struct sock *sk, int level, optname != IP_IPSEC_POLICY && optname != IP_XFRM_POLICY && !ip_mroute_opt(optname)) - err = nf_setsockopt(sk, PF_INET, optname, optval, optlen); + err = nf_setsockopt(sk, PF_INET, optname, USER_SOCKPTR(optval), + optlen); #endif return err; } diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c index 6d24b686c7f00a..f5b26ef1782001 100644 --- a/net/ipv4/netfilter/arp_tables.c +++ b/net/ipv4/netfilter/arp_tables.c @@ -1,4 +1,4 @@ -// SPDX-License-Identifier: GPL-2.0-only + /* * Packet matching code for ARP packets. * @@ -947,8 +947,7 @@ static int __do_replace(struct net *net, const char *name, return ret; } -static int do_replace(struct net *net, const void __user *user, - unsigned int len) +static int do_replace(struct net *net, sockptr_t arg, unsigned int len) { int ret; struct arpt_replace tmp; @@ -956,7 +955,7 @@ static int do_replace(struct net *net, const void __user *user, void *loc_cpu_entry; struct arpt_entry *iter; - if (copy_from_user(&tmp, user, sizeof(tmp)) != 0) + if (copy_from_sockptr(&tmp, arg, sizeof(tmp)) != 0) return -EFAULT; /* overflow check */ @@ -972,8 +971,8 @@ static int do_replace(struct net *net, const void __user *user, return -ENOMEM; loc_cpu_entry = newinfo->entries; - if (copy_from_user(loc_cpu_entry, user + sizeof(tmp), - tmp.size) != 0) { + sockptr_advance(arg, sizeof(tmp)); + if (copy_from_sockptr(loc_cpu_entry, arg, tmp.size) != 0) { ret = -EFAULT; goto free_newinfo; } @@ -1244,8 +1243,7 @@ static int translate_compat_table(struct net *net, return ret; } -static int compat_do_replace(struct net *net, void __user *user, - unsigned int len) +static int compat_do_replace(struct net *net, sockptr_t arg, unsigned int len) { int ret; struct compat_arpt_replace tmp; @@ -1253,7 +1251,7 @@ static int compat_do_replace(struct net *net, void __user *user, void *loc_cpu_entry; struct arpt_entry *iter; - if (copy_from_user(&tmp, user, sizeof(tmp)) != 0) + if (copy_from_sockptr(&tmp, arg, sizeof(tmp)) != 0) return -EFAULT; /* overflow check */ @@ -1269,7 +1267,8 @@ static int compat_do_replace(struct net *net, void __user *user, return -ENOMEM; loc_cpu_entry = newinfo->entries; - if (copy_from_user(loc_cpu_entry, user + sizeof(tmp), tmp.size) != 0) { + sockptr_advance(arg, sizeof(tmp)); + if (copy_from_sockptr(loc_cpu_entry, arg, tmp.size) != 0) { ret = -EFAULT; goto free_newinfo; } @@ -1401,7 +1400,8 @@ static int compat_get_entries(struct net *net, } #endif -static int do_arpt_set_ctl(struct sock *sk, int cmd, void __user *user, unsigned int len) +static int do_arpt_set_ctl(struct sock *sk, int cmd, sockptr_t arg, + unsigned int len) { int ret; @@ -1412,14 +1412,14 @@ static int do_arpt_set_ctl(struct sock *sk, int cmd, void __user *user, unsigned case ARPT_SO_SET_REPLACE: #ifdef CONFIG_COMPAT if (in_compat_syscall()) - ret = compat_do_replace(sock_net(sk), user, len); + ret = compat_do_replace(sock_net(sk), arg, len); else #endif - ret = do_replace(sock_net(sk), user, len); + ret = do_replace(sock_net(sk), arg, len); break; case ARPT_SO_SET_ADD_COUNTERS: - ret = do_add_counters(sock_net(sk), USER_SOCKPTR(user), len); + ret = do_add_counters(sock_net(sk), arg, len); break; default: diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c index 4697d09c98dc3e..f2a9680303d8c0 100644 --- a/net/ipv4/netfilter/ip_tables.c +++ b/net/ipv4/netfilter/ip_tables.c @@ -1102,7 +1102,7 @@ __do_replace(struct net *net, const char *name, unsigned int valid_hooks, } static int -do_replace(struct net *net, const void __user *user, unsigned int len) +do_replace(struct net *net, sockptr_t arg, unsigned int len) { int ret; struct ipt_replace tmp; @@ -1110,7 +1110,7 @@ do_replace(struct net *net, const void __user *user, unsigned int len) void *loc_cpu_entry; struct ipt_entry *iter; - if (copy_from_user(&tmp, user, sizeof(tmp)) != 0) + if (copy_from_sockptr(&tmp, arg, sizeof(tmp)) != 0) return -EFAULT; /* overflow check */ @@ -1126,8 +1126,8 @@ do_replace(struct net *net, const void __user *user, unsigned int len) return -ENOMEM; loc_cpu_entry = newinfo->entries; - if (copy_from_user(loc_cpu_entry, user + sizeof(tmp), - tmp.size) != 0) { + sockptr_advance(arg, sizeof(tmp)); + if (copy_from_sockptr(loc_cpu_entry, arg, tmp.size) != 0) { ret = -EFAULT; goto free_newinfo; } @@ -1484,7 +1484,7 @@ translate_compat_table(struct net *net, } static int -compat_do_replace(struct net *net, void __user *user, unsigned int len) +compat_do_replace(struct net *net, sockptr_t arg, unsigned int len) { int ret; struct compat_ipt_replace tmp; @@ -1492,7 +1492,7 @@ compat_do_replace(struct net *net, void __user *user, unsigned int len) void *loc_cpu_entry; struct ipt_entry *iter; - if (copy_from_user(&tmp, user, sizeof(tmp)) != 0) + if (copy_from_sockptr(&tmp, arg, sizeof(tmp)) != 0) return -EFAULT; /* overflow check */ @@ -1508,8 +1508,8 @@ compat_do_replace(struct net *net, void __user *user, unsigned int len) return -ENOMEM; loc_cpu_entry = newinfo->entries; - if (copy_from_user(loc_cpu_entry, user + sizeof(tmp), - tmp.size) != 0) { + sockptr_advance(arg, sizeof(tmp)); + if (copy_from_sockptr(loc_cpu_entry, arg, tmp.size) != 0) { ret = -EFAULT; goto free_newinfo; } @@ -1610,7 +1610,7 @@ compat_get_entries(struct net *net, struct compat_ipt_get_entries __user *uptr, #endif static int -do_ipt_set_ctl(struct sock *sk, int cmd, void __user *user, unsigned int len) +do_ipt_set_ctl(struct sock *sk, int cmd, sockptr_t arg, unsigned int len) { int ret; @@ -1621,14 +1621,14 @@ do_ipt_set_ctl(struct sock *sk, int cmd, void __user *user, unsigned int len) case IPT_SO_SET_REPLACE: #ifdef CONFIG_COMPAT if (in_compat_syscall()) - ret = compat_do_replace(sock_net(sk), user, len); + ret = compat_do_replace(sock_net(sk), arg, len); else #endif - ret = do_replace(sock_net(sk), user, len); + ret = do_replace(sock_net(sk), arg, len); break; case IPT_SO_SET_ADD_COUNTERS: - ret = do_add_counters(sock_net(sk), USER_SOCKPTR(user), len); + ret = do_add_counters(sock_net(sk), arg, len); break; default: diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c index 56a74707c61741..85892b35cff7b3 100644 --- a/net/ipv6/ipv6_sockglue.c +++ b/net/ipv6/ipv6_sockglue.c @@ -996,7 +996,8 @@ int ipv6_setsockopt(struct sock *sk, int level, int optname, /* we need to exclude all possible ENOPROTOOPTs except default case */ if (err == -ENOPROTOOPT && optname != IPV6_IPSEC_POLICY && optname != IPV6_XFRM_POLICY) - err = nf_setsockopt(sk, PF_INET6, optname, optval, optlen); + err = nf_setsockopt(sk, PF_INET6, optname, USER_SOCKPTR(optval), + optlen); #endif return err; } diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c index a787aba30e2db7..1d52957a413f4a 100644 --- a/net/ipv6/netfilter/ip6_tables.c +++ b/net/ipv6/netfilter/ip6_tables.c @@ -1119,7 +1119,7 @@ __do_replace(struct net *net, const char *name, unsigned int valid_hooks, } static int -do_replace(struct net *net, const void __user *user, unsigned int len) +do_replace(struct net *net, sockptr_t arg, unsigned int len) { int ret; struct ip6t_replace tmp; @@ -1127,7 +1127,7 @@ do_replace(struct net *net, const void __user *user, unsigned int len) void *loc_cpu_entry; struct ip6t_entry *iter; - if (copy_from_user(&tmp, user, sizeof(tmp)) != 0) + if (copy_from_sockptr(&tmp, arg, sizeof(tmp)) != 0) return -EFAULT; /* overflow check */ @@ -1143,8 +1143,8 @@ do_replace(struct net *net, const void __user *user, unsigned int len) return -ENOMEM; loc_cpu_entry = newinfo->entries; - if (copy_from_user(loc_cpu_entry, user + sizeof(tmp), - tmp.size) != 0) { + sockptr_advance(arg, sizeof(tmp)); + if (copy_from_sockptr(loc_cpu_entry, arg, tmp.size) != 0) { ret = -EFAULT; goto free_newinfo; } @@ -1493,7 +1493,7 @@ translate_compat_table(struct net *net, } static int -compat_do_replace(struct net *net, void __user *user, unsigned int len) +compat_do_replace(struct net *net, sockptr_t arg, unsigned int len) { int ret; struct compat_ip6t_replace tmp; @@ -1501,7 +1501,7 @@ compat_do_replace(struct net *net, void __user *user, unsigned int len) void *loc_cpu_entry; struct ip6t_entry *iter; - if (copy_from_user(&tmp, user, sizeof(tmp)) != 0) + if (copy_from_sockptr(&tmp, arg, sizeof(tmp)) != 0) return -EFAULT; /* overflow check */ @@ -1517,8 +1517,8 @@ compat_do_replace(struct net *net, void __user *user, unsigned int len) return -ENOMEM; loc_cpu_entry = newinfo->entries; - if (copy_from_user(loc_cpu_entry, user + sizeof(tmp), - tmp.size) != 0) { + sockptr_advance(arg, sizeof(tmp)); + if (copy_from_sockptr(loc_cpu_entry, arg, tmp.size) != 0) { ret = -EFAULT; goto free_newinfo; } @@ -1619,7 +1619,7 @@ compat_get_entries(struct net *net, struct compat_ip6t_get_entries __user *uptr, #endif static int -do_ip6t_set_ctl(struct sock *sk, int cmd, void __user *user, unsigned int len) +do_ip6t_set_ctl(struct sock *sk, int cmd, sockptr_t arg, unsigned int len) { int ret; @@ -1630,14 +1630,14 @@ do_ip6t_set_ctl(struct sock *sk, int cmd, void __user *user, unsigned int len) case IP6T_SO_SET_REPLACE: #ifdef CONFIG_COMPAT if (in_compat_syscall()) - ret = compat_do_replace(sock_net(sk), user, len); + ret = compat_do_replace(sock_net(sk), arg, len); else #endif - ret = do_replace(sock_net(sk), user, len); + ret = do_replace(sock_net(sk), arg, len); break; case IP6T_SO_SET_ADD_COUNTERS: - ret = do_add_counters(sock_net(sk), USER_SOCKPTR(user), len); + ret = do_add_counters(sock_net(sk), arg, len); break; default: diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c index 4af83f466dfc2c..bcac316addabe8 100644 --- a/net/netfilter/ipvs/ip_vs_ctl.c +++ b/net/netfilter/ipvs/ip_vs_ctl.c @@ -2434,7 +2434,7 @@ static void ip_vs_copy_udest_compat(struct ip_vs_dest_user_kern *udest, } static int -do_ip_vs_set_ctl(struct sock *sk, int cmd, void __user *user, unsigned int len) +do_ip_vs_set_ctl(struct sock *sk, int cmd, sockptr_t ptr, unsigned int len) { struct net *net = sock_net(sk); int ret; @@ -2458,7 +2458,7 @@ do_ip_vs_set_ctl(struct sock *sk, int cmd, void __user *user, unsigned int len) return -EINVAL; } - if (copy_from_user(arg, user, len) != 0) + if (copy_from_sockptr(arg, ptr, len) != 0) return -EFAULT; /* Handle daemons since they have another lock */ diff --git a/net/netfilter/nf_sockopt.c b/net/netfilter/nf_sockopt.c index 90469b1f628a8e..34afcd03b6f60e 100644 --- a/net/netfilter/nf_sockopt.c +++ b/net/netfilter/nf_sockopt.c @@ -89,7 +89,7 @@ static struct nf_sockopt_ops *nf_sockopt_find(struct sock *sk, u_int8_t pf, return ops; } -int nf_setsockopt(struct sock *sk, u_int8_t pf, int val, char __user *opt, +int nf_setsockopt(struct sock *sk, u_int8_t pf, int val, sockptr_t opt, unsigned int len) { struct nf_sockopt_ops *ops;
Pass a sockptr_t to prepare for set_fs-less handling of the kernel pointer from bpf-cgroup. Signed-off-by: Christoph Hellwig <hch@lst.de> --- include/linux/netfilter.h | 6 ++++-- net/bridge/netfilter/ebtables.c | 37 +++++++++++++++------------------ net/decnet/af_decnet.c | 3 ++- net/ipv4/ip_sockglue.c | 3 ++- net/ipv4/netfilter/arp_tables.c | 28 ++++++++++++------------- net/ipv4/netfilter/ip_tables.c | 24 ++++++++++----------- net/ipv6/ipv6_sockglue.c | 3 ++- net/ipv6/netfilter/ip6_tables.c | 24 ++++++++++----------- net/netfilter/ipvs/ip_vs_ctl.c | 4 ++-- net/netfilter/nf_sockopt.c | 2 +- 10 files changed, 68 insertions(+), 66 deletions(-)