| Message ID | 85517ec3-e3d4-1bf3-8eea-cb274face1a7@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | NFS: change sign of nfs_fh length | expand |
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?
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
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
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;
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));