Clean up: ensure svc_udp_recvfrom() is getting at least a full UDP header
to avoid potential negative intermediate values.
A similar sanity check is already used in the RPC client.
Signed-off-by: Chuck Lever <[email protected]>
---
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
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 <[email protected]>
> ---
>
> 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
>
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 <[email protected]>
>> ---
>>
>> 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
On Mon, Apr 14, 2008 at 03:55:16PM -0400, Chuck Lever wrote:
> 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."
Would a BUG() accomplish the same thing?
--b.
> 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 <[email protected]>
>>> ---
>>>
>>> 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
>
>
>
On Apr 14, 2008, at 4:54 PM, J. Bruce Fields wrote:
> On Mon, Apr 14, 2008 at 03:55:16PM -0400, Chuck Lever wrote:
>> 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."
>
> Would a BUG() accomplish the same thing?
udp_recvfrom doesn't check for underflow at all; it just stores the
difference into an unsigned variable with aplomb. I'll submit a
replacement patch that removes the check and clarifies the description.
>> 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 <[email protected]>
>>>> ---
>>>>
>>>> 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
>>
>>
>>
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com