Re: [PATCH 06/24] SUNRPC: Sanity check incoming UDP datagram lengths properly

["J. Bruce Fields" <bfields@fieldses.org>]

On Mon, Apr 14, 2008 at 12:27:23PM -0400, Chuck Lever wrote:
> Clean up: ensure svc_udp_recvfrom() is getting at least a full UDP header
> to avoid potential negative intermediate values.

Is it legal in this case for skb_recv_datagram() to return an skb
without at least that minimum length?  (And if not, should this be a
BUG()?)

> A similar sanity check is already used in the RPC client.

This one?:

	repsize = skb->len - sizeof(struct udphdr);
	if (repsize < 4) {
		dprintk("RPC:       impossible RPC reply size %d!\n", repsize);
		goto dropit;
	}

I assumed it was needed because of the rpc-level check (for the 4 bytes worth
of xid), not because it needed udp-level checking.

--b.

> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  net/sunrpc/svcsock.c |   20 +++++++++++++-------
>  1 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> index d077071..de29e7f 100644
> --- a/net/sunrpc/svcsock.c
> +++ b/net/sunrpc/svcsock.c
> @@ -431,6 +431,7 @@ static int svc_udp_recvfrom(struct svc_rqst *rqstp)
>  	} buffer;
>  	struct cmsghdr *cmh = &buffer.hdr;
>  	int		err, len;
> +	unsigned int	bytes;
>  	struct msghdr msg = {
>  		.msg_name = svc_addr(rqstp),
>  		.msg_control = cmh,
> @@ -484,8 +485,13 @@ static int svc_udp_recvfrom(struct svc_rqst *rqstp)
>  	 */
>  	svc_xprt_received(&svsk->sk_xprt);
>  
> -	len  = skb->len - sizeof(struct udphdr);
> -	rqstp->rq_arg.len = len;
> +	if (skb->len < sizeof(struct udphdr)) {
> +		dprintk("svc: bad UDP datagram length: %u\n", skb->len);
> +		skb_free_datagram(svsk->sk_sk, skb);
> +		return 0;
> +	}
> +	bytes = skb->len - sizeof(struct udphdr);
> +	rqstp->rq_arg.len = bytes;
>  
>  	rqstp->rq_prot = IPPROTO_UDP;
>  
> @@ -515,7 +521,7 @@ static int svc_udp_recvfrom(struct svc_rqst *rqstp)
>  		/* we can use it in-place */
>  		rqstp->rq_arg.head[0].iov_base = skb->data +
>  			sizeof(struct udphdr);
> -		rqstp->rq_arg.head[0].iov_len = len;
> +		rqstp->rq_arg.head[0].iov_len = bytes;
>  		if (skb_checksum_complete(skb)) {
>  			skb_free_datagram(svsk->sk_sk, skb);
>  			return 0;
> @@ -524,12 +530,12 @@ static int svc_udp_recvfrom(struct svc_rqst *rqstp)
>  	}
>  
>  	rqstp->rq_arg.page_base = 0;
> -	if (len <= rqstp->rq_arg.head[0].iov_len) {
> -		rqstp->rq_arg.head[0].iov_len = len;
> +	if (bytes <= rqstp->rq_arg.head[0].iov_len) {
> +		rqstp->rq_arg.head[0].iov_len = bytes;
>  		rqstp->rq_arg.page_len = 0;
>  		rqstp->rq_respages = rqstp->rq_pages+1;
>  	} else {
> -		rqstp->rq_arg.page_len = len - rqstp->rq_arg.head[0].iov_len;
> +		rqstp->rq_arg.page_len = bytes - rqstp->rq_arg.head[0].iov_len;
>  		rqstp->rq_respages = rqstp->rq_pages + 1 +
>  			DIV_ROUND_UP(rqstp->rq_arg.page_len, PAGE_SIZE);
>  	}
> @@ -537,7 +543,7 @@ static int svc_udp_recvfrom(struct svc_rqst *rqstp)
>  	if (serv->sv_stats)
>  		serv->sv_stats->netudpcnt++;
>  
> -	return len;
> +	return bytes;
>  }
>  
>  static int
>