Re: [PATCH] SUNRPC: Fix xdr_decode_string_inplace() mixed sign comparison

[Chuck Lever <chuck.lever@oracle.com>]

[-- Attachment #1: Type: text/plain, Size: 3015 bytes --]

Trond Myklebust wrote:
> On Wed, 2007-10-31 at 21:53 -0400, Chuck Lever wrote:
>> On Oct 31, 2007, at 3:00 PM, Trond Myklebust wrote:
>>> On Wed, 2007-10-31 at 13:06 -0400, Talpey, Thomas wrote:
>>>> This is a serious vunerability! A huge string length will always be
>>>> accepted by this code, right? Security/integrity bug, not a minor
>>>> sign cleanup IOW.
>>> Wrong! The current code is quite correct.
>>>
>>> It trusts that the caller is setting a reasonable value for maxlen,  
>>> and
>>> assumes that 'len' is the untrusted value (since it comes from the
>>> network).
>>>
>>> in the comparison
>>>
>>>   ((len = ntohl(*p++)) < maxlen)
>>>
>>> then the trusted value maxlen is the one that gets cast to an unsigned
>>> value since 'len' and 'maxlen' are both integers of the same rank (see
>>> the description of the usual binary conversions in section 6.3.4 in
>>> Harbison and Steele).
>> Whatever H&S says, the compiler flags this as a mixed sign  
>> comparison.  Thus something is not working the way you assume it is.
>>
>> [cel@ingres NFS_ALL]$ make net/sunrpc/xdr.o
>>    Using /home/cel/src/linux/NFS_ALL as source for kernel
>>    GEN     /u/cel/obj/Makefile
>>    CHK     include/linux/version.h
>>    CHK     include/linux/utsrelease.h
>>    UPD     include/linux/utsrelease.h
>>    CALL    /home/cel/src/linux/NFS_ALL/scripts/checksyscalls.sh
>>    CC      net/sunrpc/xdr.o
>> /home/cel/src/linux/NFS_ALL/net/sunrpc/xdr.c: In function  
>> xdr_decode_string_inplace:
>> /home/cel/src/linux/NFS_ALL/net/sunrpc/xdr.c:100: warning: comparison  
>> between signed and unsigned
>> [cel@ingres NFS_ALL]$
>>
>> Line 100 is precisely:
>>
>> 	if ((len = ntohl(*p++)) > maxlen)
> 
> Which is still correct according to both the old and new C standards. I
> know you've got that book at home...
> 
>> My gcc is the latest available for Fedora 7:
>>
>> gcc version 4.1.2 20070925 (Red Hat 4.1.2-27)
>>
>> I rather prefer spelling this out completely so that neither the  
>> compiler nor humans can mistake the intent of this logic.
> 
> That's fine, but please do not change the logic. The correct change is
> to replace the maxlen parameter with an unsigned int.

That's what I sent you originally.  You rejected it:

On October 26, 2007 at 14:24 -0400, Trond Myklebust said:
>> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
>> index 3d1f7cd..ff16bab 100644
>> --- a/net/sunrpc/xdr.c
>> +++ b/net/sunrpc/xdr.c
>> @@ -93,7 +93,7 @@ xdr_encode_string(__be32 *p, const char *string)
>>  }
>>  
>>  __be32 *
>> -xdr_decode_string_inplace(__be32 *p, char **sp, int *lenp, int maxlen)
>> +xdr_decode_string_inplace(__be32 *p, char **sp, int *lenp, unsigned int maxlen)
>>  {
>>  	unsigned int	len;
 >
> Nope. maxlen should be of the same type as *lenp.
> 
> Trond

Thus I now argue that both *lenp and maxlen should either be unsigned 
integers or size_t.  Negative string lengths make no sense whatsoever.

If we change both arguments, then we should also change the callers, at 
least to be consistent.

[-- Attachment #2: chuck.lever.vcf --]
[-- Type: text/x-vcard, Size: 259 bytes --]

begin:vcard
fn:Chuck Lever
n:Lever;Chuck
org:Oracle Corporation;Corporate Architecture: Linux Projects Group
adr:;;1015 Granger Avenue;Ann Arbor;MI;48104;USA
title:Principal Member of Staff
tel;work:+1 248 614 5091
x-mozilla-html:FALSE
version:2.1
end:vcard


[-- Attachment #3: Type: text/plain, Size: 314 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

[-- Attachment #4: Type: text/plain, Size: 140 bytes --]

_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs