On Apr 14, 2008, at 2:38 PM, J. Bruce Fields wrote:
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.
The server side check programmatically guarantees we get a non-
negative number when we compute "bytes." Thus we can use an unsigned
variable and eliminate a mixed sign comparison later on.
We can add a "+ sizeof(u32)" to the check if you like.
Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
---
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
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
