lkml.org 
[lkml]   [2011]   [Sep]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH net-next-2.6 ] Fix overflow of socket buffer in sunrpc
On Fri, Sep 02, 2011 at 12:41:38PM +0900, Mitsuo Hayasaka wrote:
> The sk_sndbuf and sk_rcvbuf fields of struct sock are sizes of send and
> receive socket buffers respectively, and are defined as integer.
> The sunrpc which is used in NFSD and any other applications

I'd call those "kernel subsystems" or "in-kernel applications"--it needs
to be clear that you're not talking about userspace applications.

> can change them
> via svc_sock_setbufsize(). It, however, sets them as unsigned integer and
> may cause overflow of integer. This leads to a degradation of networking
> capability.

Tracing through the callers, actually I believe they all set this to a
constant, with the single exception of nfsd, which allows the size to be
configured; but write_maxlksize already limits it to NFSSVC_MAXBLKSIZE.

So this patch looks unnecessary to me, unless I'm missing something.

If you can argue that it would be safer to check here as well, I might
consider that, but please:

- do make that argument in detail;
- especially, convince me that *this* is the right place for the
check; and
- also double-check my audit of the callers.

And include all that in the changelog.

Dropping for now.

--b.

>
> This patch adds integer-overflow check into svc_sock_setbufsize() before
> both fields are set, and limits their maximum sizes to INT_MAX.
>
> Signed-off-by: Mitsuo Hayasaka <mitsuo.hayasaka.hu@hitachi.com>
> Cc: Trond Myklebust <Trond.Myklebust@netapp.com>
> Cc: "J. Bruce Fields" <bfields@fieldses.org>
> Cc: Neil Brown <neilb@suse.de>
> Cc: "David S. Miller" <davem@davemloft.net>
> ---
>
> net/sunrpc/svcsock.c | 6 ++++++
> 1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> index 767d494..bd66775 100644
> --- a/net/sunrpc/svcsock.c
> +++ b/net/sunrpc/svcsock.c
> @@ -54,6 +54,7 @@
> #include "sunrpc.h"
>
> #define RPCDBG_FACILITY RPCDBG_SVCXPRT
> +#define MAX_SKBUFSIZ INT_MAX
>
>
> static struct svc_sock *svc_setup_socket(struct svc_serv *, struct socket *,
> @@ -435,6 +436,11 @@ static void svc_sock_setbufsize(struct socket *sock, unsigned int snd,
> * on not having CAP_SYS_RESOURCE or similar, we go direct...
> * DaveM said I could!
> */
> + if (snd > MAX_SKBUFSIZ/2)
> + snd = MAX_SKBUFSIZ/2;
> + if (rcv > MAX_SKBUFSIZ/2)
> + rcv = MAX_SKBUFSIZ/2;
> +
> lock_sock(sock->sk);
> sock->sk->sk_sndbuf = snd * 2;
> sock->sk->sk_rcvbuf = rcv * 2;
>


\
 
 \ /
  Last update: 2011-09-14 14:15    [W:0.202 / U:0.888 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site