Paranoia: Ensure a negative error value return from kernel_sendpage never
matches a large buffer length.
Signed-off-by: Chuck Lever <[email protected]>
---
net/sunrpc/svcsock.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index 6d4162b..a8ae279 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -200,7 +200,7 @@ static int svc_sendto(struct svc_rqst *rqstp, struct xdr_buf *xdr)
flags = 0;
len = kernel_sendpage(sock, rqstp->rq_respages[0], 0,
xdr->head[0].iov_len, flags);
- if (len != xdr->head[0].iov_len)
+ if (len < 0 || len != xdr->head[0].iov_len)
goto out;
slen -= xdr->head[0].iov_len;
if (slen == 0)
On Mon, Apr 14, 2008 at 12:27:08PM -0400, Chuck Lever wrote:
> Paranoia: Ensure a negative error value return from kernel_sendpage never
> matches a large buffer length.
That is a little paranoid. Absent an argument for exactly what sort of
bug could allow us to reach this point with the head iov_len in at least
the gigabytes, I'm inclined to leave this alone for simplicity's
sake....
--b.
>
> Signed-off-by: Chuck Lever <[email protected]>
> ---
>
> net/sunrpc/svcsock.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> index 6d4162b..a8ae279 100644
> --- a/net/sunrpc/svcsock.c
> +++ b/net/sunrpc/svcsock.c
> @@ -200,7 +200,7 @@ static int svc_sendto(struct svc_rqst *rqstp, struct xdr_buf *xdr)
> flags = 0;
> len = kernel_sendpage(sock, rqstp->rq_respages[0], 0,
> xdr->head[0].iov_len, flags);
> - if (len != xdr->head[0].iov_len)
> + if (len < 0 || len != xdr->head[0].iov_len)
> goto out;
> slen -= xdr->head[0].iov_len;
> if (slen == 0)
>
On Apr 14, 2008, at 1:48 PM, J. Bruce Fields wrote:
> On Mon, Apr 14, 2008 at 12:27:08PM -0400, Chuck Lever wrote:
>> Paranoia: Ensure a negative error value return from kernel_sendpage
>> never
>> matches a large buffer length.
>
> That is a little paranoid. Absent an argument for exactly what sort
> of
> bug could allow us to reach this point with the head iov_len in at
> least
> the gigabytes, I'm inclined to leave this alone for simplicity's
> sake....
I would like to document that we have noticed the mixed sign
comparison there, and that it has been made entirely safe. In certain
cases, the code as it stands does not do what it looks like it does.
I'm not recommending these changes to be pedantic, nor am I doing this
for my own health. When we leave checks like this out, what we have
is "clever" nondeterministic code rather than safe code. This is
simply good software engineering, but every one of these I've proposed
has been argued over and often rejected.
We know that humans write this code, and that humans make mistakes.
We should be doing everything we can to make this code less vulnerable
to coding mistakes. This is not about _fixing_ bugs, it's about
_preventing_ them.
>> Signed-off-by: Chuck Lever <[email protected]>
>> ---
>>
>> net/sunrpc/svcsock.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
>> index 6d4162b..a8ae279 100644
>> --- a/net/sunrpc/svcsock.c
>> +++ b/net/sunrpc/svcsock.c
>> @@ -200,7 +200,7 @@ static int svc_sendto(struct svc_rqst *rqstp,
>> struct xdr_buf *xdr)
>> flags = 0;
>> len = kernel_sendpage(sock, rqstp->rq_respages[0], 0,
>> xdr->head[0].iov_len, flags);
>> - if (len != xdr->head[0].iov_len)
>> + if (len < 0 || len != xdr->head[0].iov_len)
>> goto out;
>> slen -= xdr->head[0].iov_len;
>> if (slen == 0)
>>
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
On Mon, Apr 14, 2008 at 03:43:12PM -0400, Chuck Lever wrote:
> On Apr 14, 2008, at 1:48 PM, J. Bruce Fields wrote:
>> On Mon, Apr 14, 2008 at 12:27:08PM -0400, Chuck Lever wrote:
>>> Paranoia: Ensure a negative error value return from kernel_sendpage
>>> never
>>> matches a large buffer length.
>>
>> That is a little paranoid. Absent an argument for exactly what sort
>> of
>> bug could allow us to reach this point with the head iov_len in at
>> least
>> the gigabytes, I'm inclined to leave this alone for simplicity's
>> sake....
>
> I would like to document that we have noticed the mixed sign comparison
> there, and that it has been made entirely safe. In certain cases, the
> code as it stands does not do what it looks like it does.
>
> I'm not recommending these changes to be pedantic, nor am I doing this
> for my own health. When we leave checks like this out, what we have is
> "clever" nondeterministic code rather than safe code.
OK, I can buy the argument that it's a little clever and non-idiomatic
to skip the explicit check for the error case.
> This is simply
> good software engineering, but every one of these I've proposed has been
> argued over and often rejected.
>
> We know that humans write this code, and that humans make mistakes. We
> should be doing everything we can to make this code less vulnerable to
> coding mistakes. This is not about _fixing_ bugs, it's about
> _preventing_ them.
I understand that, but for me there's a problem of documentation. It
helps me, as I read the code, if I can distinguish between failures that
our code is designed to handle (a buggy or malicious nfs client, a
failed disk, whatever) from failures that it's not designed to handle
(memory corruption). So for the latter, my knee-jerk reaction is "why
isn't that a BUG_ON (or at least a WARN_ON)?"
But admittedly in this particular case, that's not really a problem--I
can't really see thinking "hey, is this code suggesting that it's
possible for iov_len to be greater than 2^31 at this point?".
--b.
>
>>> Signed-off-by: Chuck Lever <[email protected]>
>>> ---
>>>
>>> net/sunrpc/svcsock.c | 2 +-
>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
>>> index 6d4162b..a8ae279 100644
>>> --- a/net/sunrpc/svcsock.c
>>> +++ b/net/sunrpc/svcsock.c
>>> @@ -200,7 +200,7 @@ static int svc_sendto(struct svc_rqst *rqstp,
>>> struct xdr_buf *xdr)
>>> flags = 0;
>>> len = kernel_sendpage(sock, rqstp->rq_respages[0], 0,
>>> xdr->head[0].iov_len, flags);
>>> - if (len != xdr->head[0].iov_len)
>>> + if (len < 0 || len != xdr->head[0].iov_len)
>>> goto out;
>>> slen -= xdr->head[0].iov_len;
>>> if (slen == 0)
>>>
>
> --
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
>
>
>