NFS: change sign of nfs_fh length
diff mbox series

Message ID 85517ec3-e3d4-1bf3-8eea-cb274face1a7@redhat.com
State New
Headers show
Series
  • NFS: change sign of nfs_fh length
Related show

Commit Message

Frank Sorenson Oct. 23, 2018, 3:34 p.m. UTC
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 <sorenson@redhat.com>
---
 fs/nfs/nfs4xdr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

Comments

Matthew Wilcox Oct. 23, 2018, 3:40 p.m. UTC | #1
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 <sorenson@redhat.com>

Is this a cleanup or does it fix a user-visible bug?
Trond Myklebust Oct. 23, 2018, 4:09 p.m. UTC | #2
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 <sorenson@redhat.com>
> 
> 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
Benjamin Coddington Oct. 23, 2018, 5:10 p.m. UTC | #3
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 <sorenson@redhat.com>
>>
>> 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

Patch
diff mbox series

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;