| Message ID | 20090206.005034.193709323.davem@davemloft.net |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, Feb 06, 2009 at 12:50:34AM -0800, David Miller wrote: > > ipv6: Disallow rediculious flowlabel option sizes. > > Just like PKTINFO, limit the options area to 64K. > > Based upon report by Eric Sesterhenn and analysis by > Roland Dreier. > > Signed-off-by: David S. Miller <davem@davemloft.net> This should eliminate the backtrace as reported, but it'll just show up somewhere else. In particular, the control message (see sys_sendmsg) itself is copied to kernel memory via kmalloc and it's limited to INT_MAX :) (The option we're discussing here is stored in the control message). We either need to vmalloc it or have everyone read the user memory directly. Cheers,
From: Herbert Xu <herbert@gondor.apana.org.au> Date: Fri, 6 Feb 2009 19:54:06 +1100 > This should eliminate the backtrace as reported, but it'll just > show up somewhere else. In particular, the control message (see > sys_sendmsg) itself is copied to kernel memory via kmalloc and > it's limited to INT_MAX :) Can you be more specific, do you mean this is happening via datagram_send_ctl()? If we limit how big these txoption blobs can be, at all sites where we parse instances the user gives us, I can't see any such problems. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 06, 2009 at 01:05:59AM -0800, David Miller wrote: > From: Herbert Xu <herbert@gondor.apana.org.au> > Date: Fri, 6 Feb 2009 19:54:06 +1100 > > > This should eliminate the backtrace as reported, but it'll just > > show up somewhere else. In particular, the control message (see > > sys_sendmsg) itself is copied to kernel memory via kmalloc and > > it's limited to INT_MAX :) > > Can you be more specific, do you mean this is happening > via datagram_send_ctl()? Well the data that led to the back trace came from a cmsg, I was merely pointing out that the cmsg itself is kmalloced and copied by sys_sendmsg and it's only limited to INT_MAX. So even if you throw away the message in IPv6, sys_sendmsg will still do an even bigger allocation on that message which is just as likely to fail. Cheers,
From: Herbert Xu <herbert@gondor.apana.org.au> Date: Fri, 6 Feb 2009 20:22:18 +1100 > Well the data that led to the back trace came from a cmsg, I was > merely pointing out that the cmsg itself is kmalloced and copied > by sys_sendmsg and it's only limited to INT_MAX. Actually, it's limited to sysctl_optmem_max :-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 06, 2009 at 01:27:33AM -0800, David Miller wrote: > > Actually, it's limited to sysctl_optmem_max :-) Hiding the check in sock_kmalloc, clever :) In that case we don't have a problem. But we should probably bring the check down to sysctl_optmem_max in ip6_flowlabel.c too since allocating 64K is still quite likely to fail and warn. Thanks,
On Fri, Feb 06, 2009 at 09:27:10PM +1100, Herbert Xu wrote: > > But we should probably bring the check down to sysctl_optmem_max > in ip6_flowlabel.c too since allocating 64K is still quite likely > to fail and warn. In fact, we should probably just use sock_kmalloc in these places. Cheers,
From: Herbert Xu <herbert@gondor.apana.org.au> Date: Fri, 6 Feb 2009 21:27:10 +1100 > On Fri, Feb 06, 2009 at 01:27:33AM -0800, David Miller wrote: > > > > Actually, it's limited to sysctl_optmem_max :-) > > Hiding the check in sock_kmalloc, clever :) In that case we don't > have a problem. > > But we should probably bring the check down to sysctl_optmem_max > in ip6_flowlabel.c too since allocating 64K is still quite likely > to fail and warn. Good idea. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Herbert Xu <herbert@gondor.apana.org.au> Date: Fri, 6 Feb 2009 21:34:43 +1100 > On Fri, Feb 06, 2009 at 09:27:10PM +1100, Herbert Xu wrote: > > > > But we should probably bring the check down to sysctl_optmem_max > > in ip6_flowlabel.c too since allocating 64K is still quite likely > > to fail and warn. > > In fact, we should probably just use sock_kmalloc in these places. Very likely we could get away with that. But some apps might start breaking since this would now charge the socket and we could hit the limits whereas before we wouldn't. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
David Miller <davem@davemloft.net> wrote: > > But some apps might start breaking since this would now > charge the socket and we could hit the limits whereas > before we wouldn't. Yes we should definitely test this carefully. However, I think it is the right thing to do since we shouldn't leave holes where the user can allocate kernel memory that is uncapped. In fact, a number of spots in IPv6 already use sock_kmalloc for these objects anyway (e.g., ipv6_dup_options in exthdrs.c) so there is no fundamental reason why this limit can't be imposed. Cheers,
From: Herbert Xu <herbert@gondor.apana.org.au> Date: Mon, 9 Feb 2009 16:03:54 +1100 > In fact, a number of spots in IPv6 already use sock_kmalloc > for these objects anyway (e.g., ipv6_dup_options in exthdrs.c) > so there is no fundamental reason why this limit can't be imposed. Indeed, we are consistently inconsistent here. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Herbert Xu <herbert@gondor.apana.org.au> Date: Mon, 9 Feb 2009 16:03:54 +1100 > David Miller <davem@davemloft.net> wrote: > > > > But some apps might start breaking since this would now > > charge the socket and we could hit the limits whereas > > before we wouldn't. > > Yes we should definitely test this carefully. > > However, I think it is the right thing to do since we shouldn't > leave holes where the user can allocate kernel memory that is > uncapped. > > In fact, a number of spots in IPv6 already use sock_kmalloc > for these objects anyway (e.g., ipv6_dup_options in exthdrs.c) > so there is no fundamental reason why this limit can't be imposed. Actually it turns out we can't do that here. This flow label object is global, rather than specifically associated with a given socket. See net/ipv6/ip6_flowlabel.c:fl_{create,intern}() and how those are used. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 24, 2009 at 11:44:01PM -0800, David Miller wrote: > > Actually it turns out we can't do that here. This flow label object > is global, rather than specifically associated with a given socket. > > See net/ipv6/ip6_flowlabel.c:fl_{create,intern}() and how those > are used. They're global but that doesn't stop us from charging it once for each socket that uses it (isn't that what Microsoft does for Windows :) The fact is right now we have an entity that a user can create willy nilly with no limits. That can't be good. Cheers,
diff --git a/net/ipv6/ip6_flowlabel.c b/net/ipv6/ip6_flowlabel.c index c62dd24..7712578 100644 --- a/net/ipv6/ip6_flowlabel.c +++ b/net/ipv6/ip6_flowlabel.c @@ -323,17 +323,21 @@ static struct ip6_flowlabel * fl_create(struct net *net, struct in6_flowlabel_req *freq, char __user *optval, int optlen, int *err_p) { - struct ip6_flowlabel *fl; + struct ip6_flowlabel *fl = NULL; int olen; int addr_type; int err; + olen = optlen - CMSG_ALIGN(sizeof(*freq)); + err = -EINVAL; + if (olen > 64 * 1024) + goto done; + err = -ENOMEM; fl = kzalloc(sizeof(*fl), GFP_KERNEL); if (fl == NULL) goto done; - olen = optlen - CMSG_ALIGN(sizeof(*freq)); if (olen > 0) { struct msghdr msg; struct flowi flowi;