NFSv4: Fix buffer overflow checking in __nfs4_get_acl_uncached

Pass the checks made by decode_getacl back to __nfs4_get_acl_uncached
so that it knows if the acl has been truncated.

The current overflow checking is broken, resulting in Oopses on
user-triggered nfs4_getfacl calls, and is opaque to the point
where several attempts at fixing it have failed.
This patch tries to clean up the code in addition to fixing the
Oopses by ensuring that the overflow checks are performed in
a single place (decode_getacl). If the overflow check failed,
we will still be able to report the acl length, but at least
we will no longer attempt to cache the acl or copy the
truncated contents to user space.

Reported-by: Sachin Prabhu <sprabhu@redhat.com>
Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
Tested-by: Sachin Prabhu <sprabhu@redhat.com>
This commit is contained in:
Trond Myklebust 2012-08-26 11:44:43 -07:00
parent 21f498c2f7
commit 1f1ea6c2d9
3 changed files with 18 additions and 29 deletions

View File

@ -3739,7 +3739,7 @@ static void nfs4_write_cached_acl(struct inode *inode, struct page **pages, size
struct nfs4_cached_acl *acl; struct nfs4_cached_acl *acl;
size_t buflen = sizeof(*acl) + acl_len; size_t buflen = sizeof(*acl) + acl_len;
if (pages && buflen <= PAGE_SIZE) { if (buflen <= PAGE_SIZE) {
acl = kmalloc(buflen, GFP_KERNEL); acl = kmalloc(buflen, GFP_KERNEL);
if (acl == NULL) if (acl == NULL)
goto out; goto out;
@ -3784,7 +3784,6 @@ static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t bu
}; };
unsigned int npages = DIV_ROUND_UP(buflen, PAGE_SIZE); unsigned int npages = DIV_ROUND_UP(buflen, PAGE_SIZE);
int ret = -ENOMEM, i; int ret = -ENOMEM, i;
size_t acl_len = 0;
/* As long as we're doing a round trip to the server anyway, /* As long as we're doing a round trip to the server anyway,
* let's be prepared for a page of acl data. */ * let's be prepared for a page of acl data. */
@ -3807,11 +3806,6 @@ static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t bu
args.acl_len = npages * PAGE_SIZE; args.acl_len = npages * PAGE_SIZE;
args.acl_pgbase = 0; args.acl_pgbase = 0;
/* Let decode_getfacl know not to fail if the ACL data is larger than
* the page we send as a guess */
if (buf == NULL)
res.acl_flags |= NFS4_ACL_LEN_REQUEST;
dprintk("%s buf %p buflen %zu npages %d args.acl_len %zu\n", dprintk("%s buf %p buflen %zu npages %d args.acl_len %zu\n",
__func__, buf, buflen, npages, args.acl_len); __func__, buf, buflen, npages, args.acl_len);
ret = nfs4_call_sync(NFS_SERVER(inode)->client, NFS_SERVER(inode), ret = nfs4_call_sync(NFS_SERVER(inode)->client, NFS_SERVER(inode),
@ -3819,20 +3813,19 @@ static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t bu
if (ret) if (ret)
goto out_free; goto out_free;
acl_len = res.acl_len; /* Handle the case where the passed-in buffer is too short */
if (acl_len > args.acl_len) if (res.acl_flags & NFS4_ACL_TRUNC) {
nfs4_write_cached_acl(inode, NULL, 0, acl_len); /* Did the user only issue a request for the acl length? */
else if (buf == NULL)
nfs4_write_cached_acl(inode, pages, res.acl_data_offset, goto out_ok;
acl_len);
if (buf) {
ret = -ERANGE; ret = -ERANGE;
if (acl_len > buflen)
goto out_free; goto out_free;
_copy_from_pages(buf, pages, res.acl_data_offset,
acl_len);
} }
ret = acl_len; nfs4_write_cached_acl(inode, pages, res.acl_data_offset, res.acl_len);
if (buf)
_copy_from_pages(buf, pages, res.acl_data_offset, res.acl_len);
out_ok:
ret = res.acl_len;
out_free: out_free:
for (i = 0; i < npages; i++) for (i = 0; i < npages; i++)
if (pages[i]) if (pages[i])

View File

@ -5072,18 +5072,14 @@ static int decode_getacl(struct xdr_stream *xdr, struct rpc_rqst *req,
* are stored with the acl data to handle the problem of * are stored with the acl data to handle the problem of
* variable length bitmaps.*/ * variable length bitmaps.*/
res->acl_data_offset = xdr_stream_pos(xdr) - pg_offset; res->acl_data_offset = xdr_stream_pos(xdr) - pg_offset;
/* We ignore &savep and don't do consistency checks on
* the attr length. Let userspace figure it out.... */
res->acl_len = attrlen; res->acl_len = attrlen;
if (attrlen > (xdr->nwords << 2)) {
if (res->acl_flags & NFS4_ACL_LEN_REQUEST) { /* Check for receive buffer overflow */
/* getxattr interface called with a NULL buf */ if (res->acl_len > (xdr->nwords << 2) ||
goto out; res->acl_len + res->acl_data_offset > xdr->buf->page_len) {
} res->acl_flags |= NFS4_ACL_TRUNC;
dprintk("NFS: acl reply: attrlen %u > page_len %u\n", dprintk("NFS: acl reply: attrlen %u > page_len %u\n",
attrlen, xdr->nwords << 2); attrlen, xdr->nwords << 2);
return -EINVAL;
} }
} else } else
status = -EOPNOTSUPP; status = -EOPNOTSUPP;

View File

@ -652,7 +652,7 @@ struct nfs_getaclargs {
}; };
/* getxattr ACL interface flags */ /* getxattr ACL interface flags */
#define NFS4_ACL_LEN_REQUEST 0x0001 /* zero length getxattr buffer */ #define NFS4_ACL_TRUNC 0x0001 /* ACL was truncated */
struct nfs_getaclres { struct nfs_getaclres {
size_t acl_len; size_t acl_len;
size_t acl_data_offset; size_t acl_data_offset;