| Message ID | 20240409120741.3538135-1-edumazet@google.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [net] netfilter: complete validation of user input | expand |
On Tue, Apr 09, 2024 at 12:07:41PM +0000, Eric Dumazet wrote: > In my recent commit, I missed that do_replace() handlers > use copy_from_sockptr() (which I fixed), followed > by unsafe copy_from_sockptr_offset() calls. I forgot too to git grep away from net/netfilter/ folder for some reason. > In all functions, we can perform the @optlen validation > before even calling xt_alloc_table_info() with the following > check: > > if ((u64)optlen < (u64)tmp.size + sizeof(tmp)) > return -EINVAL; Thanks for this fix. > Fixes: 0c83842df40f ("netfilter: validate user input for expected length") > Reported-by: syzbot <syzkaller@googlegroups.com> Reviewed-by: Pablo Neira Ayuso <pablo@netfilter.org> > Signed-off-by: Eric Dumazet <edumazet@google.com> > --- > net/ipv4/netfilter/arp_tables.c | 4 ++++ > net/ipv4/netfilter/ip_tables.c | 4 ++++ > net/ipv6/netfilter/ip6_tables.c | 4 ++++ > 3 files changed, 12 insertions(+) > > diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c > index b150c9929b12e86219a55c77da480e0c538b3449..14365b20f1c5c09964dd7024060116737f22cb63 100644 > --- a/net/ipv4/netfilter/arp_tables.c > +++ b/net/ipv4/netfilter/arp_tables.c > @@ -966,6 +966,8 @@ static int do_replace(struct net *net, sockptr_t arg, unsigned int len) > return -ENOMEM; > if (tmp.num_counters == 0) > return -EINVAL; > + if ((u64)len < (u64)tmp.size + sizeof(tmp)) > + return -EINVAL; > > tmp.name[sizeof(tmp.name)-1] = 0; > > @@ -1266,6 +1268,8 @@ static int compat_do_replace(struct net *net, sockptr_t arg, unsigned int len) > return -ENOMEM; > if (tmp.num_counters == 0) > return -EINVAL; > + if ((u64)len < (u64)tmp.size + sizeof(tmp)) > + return -EINVAL; > > tmp.name[sizeof(tmp.name)-1] = 0; > > diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c > index 487670759578168c5ff53bce6642898fc41936b3..fe89a056eb06c43743b2d7449e59f4e9360ba223 100644 > --- a/net/ipv4/netfilter/ip_tables.c > +++ b/net/ipv4/netfilter/ip_tables.c > @@ -1118,6 +1118,8 @@ do_replace(struct net *net, sockptr_t arg, unsigned int len) > return -ENOMEM; > if (tmp.num_counters == 0) > return -EINVAL; > + if ((u64)len < (u64)tmp.size + sizeof(tmp)) > + return -EINVAL; > > tmp.name[sizeof(tmp.name)-1] = 0; > > @@ -1504,6 +1506,8 @@ compat_do_replace(struct net *net, sockptr_t arg, unsigned int len) > return -ENOMEM; > if (tmp.num_counters == 0) > return -EINVAL; > + if ((u64)len < (u64)tmp.size + sizeof(tmp)) > + return -EINVAL; > > tmp.name[sizeof(tmp.name)-1] = 0; > > diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c > index 636b360311c5365fba2330f6ca2f7f1b6dd1363e..131f7bb2110d3a08244c6da40ff9be45a2be711b 100644 > --- a/net/ipv6/netfilter/ip6_tables.c > +++ b/net/ipv6/netfilter/ip6_tables.c > @@ -1135,6 +1135,8 @@ do_replace(struct net *net, sockptr_t arg, unsigned int len) > return -ENOMEM; > if (tmp.num_counters == 0) > return -EINVAL; > + if ((u64)len < (u64)tmp.size + sizeof(tmp)) > + return -EINVAL; > > tmp.name[sizeof(tmp.name)-1] = 0; > > @@ -1513,6 +1515,8 @@ compat_do_replace(struct net *net, sockptr_t arg, unsigned int len) > return -ENOMEM; > if (tmp.num_counters == 0) > return -EINVAL; > + if ((u64)len < (u64)tmp.size + sizeof(tmp)) > + return -EINVAL; > > tmp.name[sizeof(tmp.name)-1] = 0; > > -- > 2.44.0.478.gd926399ef9-goog >
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Tue, 9 Apr 2024 12:07:41 +0000 you wrote: > In my recent commit, I missed that do_replace() handlers > use copy_from_sockptr() (which I fixed), followed > by unsafe copy_from_sockptr_offset() calls. > > In all functions, we can perform the @optlen validation > before even calling xt_alloc_table_info() with the following > check: > > [...] Here is the summary with links: - [net] netfilter: complete validation of user input https://git.kernel.org/netdev/net/c/65acf6e0501a You are awesome, thank you!
diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c index b150c9929b12e86219a55c77da480e0c538b3449..14365b20f1c5c09964dd7024060116737f22cb63 100644 --- a/net/ipv4/netfilter/arp_tables.c +++ b/net/ipv4/netfilter/arp_tables.c @@ -966,6 +966,8 @@ static int do_replace(struct net *net, sockptr_t arg, unsigned int len) return -ENOMEM; if (tmp.num_counters == 0) return -EINVAL; + if ((u64)len < (u64)tmp.size + sizeof(tmp)) + return -EINVAL; tmp.name[sizeof(tmp.name)-1] = 0; @@ -1266,6 +1268,8 @@ static int compat_do_replace(struct net *net, sockptr_t arg, unsigned int len) return -ENOMEM; if (tmp.num_counters == 0) return -EINVAL; + if ((u64)len < (u64)tmp.size + sizeof(tmp)) + return -EINVAL; tmp.name[sizeof(tmp.name)-1] = 0; diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c index 487670759578168c5ff53bce6642898fc41936b3..fe89a056eb06c43743b2d7449e59f4e9360ba223 100644 --- a/net/ipv4/netfilter/ip_tables.c +++ b/net/ipv4/netfilter/ip_tables.c @@ -1118,6 +1118,8 @@ do_replace(struct net *net, sockptr_t arg, unsigned int len) return -ENOMEM; if (tmp.num_counters == 0) return -EINVAL; + if ((u64)len < (u64)tmp.size + sizeof(tmp)) + return -EINVAL; tmp.name[sizeof(tmp.name)-1] = 0; @@ -1504,6 +1506,8 @@ compat_do_replace(struct net *net, sockptr_t arg, unsigned int len) return -ENOMEM; if (tmp.num_counters == 0) return -EINVAL; + if ((u64)len < (u64)tmp.size + sizeof(tmp)) + return -EINVAL; tmp.name[sizeof(tmp.name)-1] = 0; diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c index 636b360311c5365fba2330f6ca2f7f1b6dd1363e..131f7bb2110d3a08244c6da40ff9be45a2be711b 100644 --- a/net/ipv6/netfilter/ip6_tables.c +++ b/net/ipv6/netfilter/ip6_tables.c @@ -1135,6 +1135,8 @@ do_replace(struct net *net, sockptr_t arg, unsigned int len) return -ENOMEM; if (tmp.num_counters == 0) return -EINVAL; + if ((u64)len < (u64)tmp.size + sizeof(tmp)) + return -EINVAL; tmp.name[sizeof(tmp.name)-1] = 0; @@ -1513,6 +1515,8 @@ compat_do_replace(struct net *net, sockptr_t arg, unsigned int len) return -ENOMEM; if (tmp.num_counters == 0) return -EINVAL; + if ((u64)len < (u64)tmp.size + sizeof(tmp)) + return -EINVAL; tmp.name[sizeof(tmp.name)-1] = 0;
In my recent commit, I missed that do_replace() handlers use copy_from_sockptr() (which I fixed), followed by unsafe copy_from_sockptr_offset() calls. In all functions, we can perform the @optlen validation before even calling xt_alloc_table_info() with the following check: if ((u64)optlen < (u64)tmp.size + sizeof(tmp)) return -EINVAL; Fixes: 0c83842df40f ("netfilter: validate user input for expected length") Reported-by: syzbot <syzkaller@googlegroups.com> Signed-off-by: Eric Dumazet <edumazet@google.com> --- net/ipv4/netfilter/arp_tables.c | 4 ++++ net/ipv4/netfilter/ip_tables.c | 4 ++++ net/ipv6/netfilter/ip6_tables.c | 4 ++++ 3 files changed, 12 insertions(+)