svn commit: r328479 - in head/sys: fs/ext2fs ufs/ufs

[Pedro Giffuni]

On 01/27/18 11:03, Konstantin Belousov wrote:
> On Sat, Jan 27, 2018 at 03:33:52PM +0000, Pedro F. Giffuni wrote:
>> Author: pfg
>> Date: Sat Jan 27 15:33:52 2018
>> New Revision: 328479
>> URL: https://svnweb.freebsd.org/changeset/base/328479
>>
>> Log:
>>    {ext2|ufs}_readdir: Set limit on valid ncookies values.
>>    
>>    Sanitize the values that will be assigned to ncookies so that we ensure
>>    they are sane and we can handle them.
>>    
>>    Let ncookies signed as it was before r328346. The valid range is such
>>    that unsigned values are not required and we are not able to avoid at
>>    least one cast anyways.
>>    
>>    Hinted by:	bde
>>
>> Modified:
>>    head/sys/fs/ext2fs/ext2_lookup.c
>>    head/sys/ufs/ufs/ufs_vnops.c
>>
>> Modified: head/sys/fs/ext2fs/ext2_lookup.c
>> ==============================================================================
>> --- head/sys/fs/ext2fs/ext2_lookup.c	Sat Jan 27 13:46:55 2018	(r328478)
>> +++ head/sys/fs/ext2fs/ext2_lookup.c	Sat Jan 27 15:33:52 2018	(r328479)
>> @@ -145,14 +145,18 @@ ext2_readdir(struct vop_readdir_args *ap)
>>   	off_t offset, startoffset;
>>   	size_t readcnt, skipcnt;
>>   	ssize_t startresid;
>> -	u_int ncookies;
>> +	int ncookies;
>>   	int DIRBLKSIZ = VTOI(ap->a_vp)->i_e2fs->e2fs_bsize;
>>   	int error;
>>   
>>   	if (uio->uio_offset < 0)
>>   		return (EINVAL);
>>   	ip = VTOI(vp);
>> +	if (uio->uio_resid < 0)
>> +		uio->uio_resid = 0;
>>   	if (ap->a_ncookies != NULL) {
>> +		if (uio->uio_resid > MAXPHYS)
>> +			uio->uio_resid = MAXPHYS;
>>   		ncookies = uio->uio_resid;
>>   		if (uio->uio_offset >= ip->i_size)
>>   			ncookies = 0;
>>
>> Modified: head/sys/ufs/ufs/ufs_vnops.c
>> ==============================================================================
>> --- head/sys/ufs/ufs/ufs_vnops.c	Sat Jan 27 13:46:55 2018	(r328478)
>> +++ head/sys/ufs/ufs/ufs_vnops.c	Sat Jan 27 15:33:52 2018	(r328479)
>> @@ -2170,7 +2170,7 @@ ufs_readdir(ap)
>>   	off_t offset, startoffset;
>>   	size_t readcnt, skipcnt;
>>   	ssize_t startresid;
>> -	u_int ncookies;
>> +	int ncookies;
>>   	int error;
>>   
>>   	if (uio->uio_offset < 0)
>> @@ -2178,7 +2178,11 @@ ufs_readdir(ap)
>>   	ip = VTOI(vp);
>>   	if (ip->i_effnlink == 0)
>>   		return (0);
>> +	if (uio->uio_resid < 0)
>> +		uio->uio_resid = 0;
>>   	if (ap->a_ncookies != NULL) {
>> +		if (uio->uio_resid > MAXPHYS)
>> +			uio->uio_resid = MAXPHYS;
> You just break nfs server.
>
> Look at nfsrvd_readdir() call to VOP_READDIR.  Almost all code which calls
> VOP_READDIR() memoize the value of uio_resid before and after the call and
> interpret the difference as the amount of data returned.
>
> I said above that only nfs server is broken, because only the server uses
> cookies, otherwise your patch would break everything.
Ugh, yes .. I completely missed the fact that uio is a pointer to the 
real thing, not a local copy.
This still should never go off limits but it is certainly wrong.

I reverted it sorry.

Pedro.