2011-03-28 08:22:43

by Dan Carpenter

Subject: smatch stuff: returning signed values in nfs_negotiate_security()

Hi Brian,

7ebb931598 "NFS: use secinfo when crossing mountpoints" has a signed vs
unsigned bug.

fs/nfs/namespace.c +189 nfs_lookup_with_sec(10)
warn: unsigned 'flavor' is never less than zero.

161 page = alloc_page(GFP_KERNEL);
162 if (!page) {
163 status = -ENOMEM;
^^^^^^^^^^^^^^^^
Btw. this gets over written after we goto out.

164 goto out;
165 }
166 flavors = page_address(page);
167 status = secinfo(parent->d_inode, &dentry->d_name, flavors);
168 flavor = nfs_find_best_sec(flavors, dentry->d_inode);
169 put_page(page);
170 }
171
172 return flavor;
173
174 out:
175 status = -ENOMEM;
^^^^^^^^^^^^^^^^
Here.

More importantly, the caller is expecting an unsigned return value. The
check for negative returns in nfs_lookup_with_sec() doesn't work.

176 return status;

regards,
dan carpenter

2011-03-28 20:59:57

by Anna Schumaker

Subject: Re: smatch stuff: returning signed values in nfs_negotiate_security()

On 03/28/2011 04:22 AM, Dan Carpenter wrote:
> Hi Brian,
>
> 7ebb931598 "NFS: use secinfo when crossing mountpoints" has a signed vs
> unsigned bug.

Thanks for pointing this out. I'll fix this up.

-Bryan

>
> fs/nfs/namespace.c +189 nfs_lookup_with_sec(10)
> warn: unsigned 'flavor' is never less than zero.
>
> 161 page = alloc_page(GFP_KERNEL);
> 162 if (!page) {
> 163 status = -ENOMEM;
> ^^^^^^^^^^^^^^^^
> Btw. this gets over written after we goto out.
>
> 164 goto out;
> 165 }
> 166 flavors = page_address(page);
> 167 status = secinfo(parent->d_inode, &dentry->d_name, flavors);
> 168 flavor = nfs_find_best_sec(flavors, dentry->d_inode);
> 169 put_page(page);
> 170 }
> 171
> 172 return flavor;
> 173
> 174 out:
> 175 status = -ENOMEM;
> ^^^^^^^^^^^^^^^^
> Here.
>
> More importantly, the caller is expecting an unsigned return value. The
> check for negative returns in nfs_lookup_with_sec() doesn't work.
>
> 176 return status;
>
> regards,
> dan carpenter