2018-10-23 15:35:03

by Frank Sorenson

Subject: [PATCH] NFS: change sign of nfs_fh length

The filehandle has a length which is defined as a 32-bit
"unsigned integer". Change sign of the length appropriately.

Signed-off-by: Frank Sorenson <[email protected]>
---
fs/nfs/nfs4xdr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index b7bde12d8cd5..2fc8f6fa25e4 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -3516,7 +3516,7 @@ static int decode_attr_exclcreat_supported(struct
xdr_stream *xdr,
static int decode_attr_filehandle(struct xdr_stream *xdr, uint32_t
*bitmap, struct nfs_fh *fh)
{
__be32 *p;
- int len;
+ u32 len;

if (fh != NULL)
memset(fh, 0, sizeof(*fh));
--
2.13.6

2018-10-23 15:40:33

by Matthew Wilcox

Subject: Re: [PATCH] NFS: change sign of nfs_fh length

On Tue, Oct 23, 2018 at 10:34:57AM -0500, Frank Sorenson wrote:
>
> The filehandle has a length which is defined as a 32-bit
> "unsigned integer". Change sign of the length appropriately.
>
> Signed-off-by: Frank Sorenson <[email protected]>

Is this a cleanup or does it fix a user-visible bug?

2018-10-23 16:09:45

by Trond Myklebust

Subject: Re: [PATCH] NFS: change sign of nfs_fh length

On Tue, 2018-10-23 at 08:40 -0700, Matthew Wilcox wrote:
> On Tue, Oct 23, 2018 at 10:34:57AM -0500, Frank Sorenson wrote:
> >
> > The filehandle has a length which is defined as a 32-bit
> > "unsigned integer". Change sign of the length appropriately.
> >
> > Signed-off-by: Frank Sorenson <[email protected]>
>
> Is this a cleanup or does it fix a user-visible bug?

It fixes the following comparison:

if (len > NFS4_FHSIZE)
return -EIO;

but in practice, the next line should always catch the buffer overflow
when len is negative:

p = xdr_inline_decode(xdr, len);
if (unlikely(!p))
goto out_overflow;

That said, it is nice to be redundant, so I'm happy to take the patch.

Frank, in the future can you please Cc: the maintainers directly on
your patches? I missed this one completely because my mail filter
directed it to my 'linux-fsdevel' inbox rather than 'linux-nfs'...

Thanks,
Trond

--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]

2018-10-23 17:11:04

by Benjamin Coddington

Subject: Re: [PATCH] NFS: change sign of nfs_fh length

On 23 Oct 2018, at 12:09, Trond Myklebust wrote:

> On Tue, 2018-10-23 at 08:40 -0700, Matthew Wilcox wrote:
>> On Tue, Oct 23, 2018 at 10:34:57AM -0500, Frank Sorenson wrote:
>>>
>>> The filehandle has a length which is defined as a 32-bit
>>> "unsigned integer". Change sign of the length appropriately.
>>>
>>> Signed-off-by: Frank Sorenson <[email protected]>
>>
>> Is this a cleanup or does it fix a user-visible bug?
>
> It fixes the following comparison:
>
> if (len > NFS4_FHSIZE)
> return -EIO;
>
> but in practice, the next line should always catch the buffer overflow
> when len is negative:
>
> p = xdr_inline_decode(xdr, len);
> if (unlikely(!p))
> goto out_overflow;

Maybe I am missing something, but if we're depending on:

static __be32 * __xdr_inline_decode(struct xdr_stream *xdr, size_t nbytes)
{
unsigned int nwords = XDR_QUADLEN(nbytes);
__be32 *p = xdr->p;
__be32 *q = p + nwords;

if (unlikely(nwords > xdr->nwords || q > xdr->end || q < p))
return NULL;

and nbytes is 0xffffffff, then nwords ends up being 0.. so this if statement
could easily miss it.

Ben