DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 42B6E20671
From:   "Benjamin Coddington" <bcodding@redhat.com>
To:     "Trond Myklebust" <trondmy@hammerspace.com>
Cc:     willy@infradead.org, sorenson@redhat.com,
        linux-nfs@vger.kernel.org, linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH] NFS: change sign of nfs_fh length
Date:   Tue, 23 Oct 2018 13:10:59 -0400
Message-ID: <6C36BA5A-94EC-4DEE-A435-5913D822CF92@redhat.com>
In-Reply-To: <c3ecb78f39487792cf01f868f2ee8521237b3a33.camel@hammerspace.com>
References: <85517ec3-e3d4-1bf3-8eea-cb274face1a7@redhat.com>
 <20181023154031.GF20085@bombadil.infradead.org>
 <c3ecb78f39487792cf01f868f2ee8521237b3a33.camel@hammerspace.com>
MIME-Version: 1.0
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

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