Quoting James Morris ([email protected]):
...
> +int nfs3_proc_getxattr(struct inode *inode, const char *namespace,
> + const char *name, void *value, size_t size)
> +{
...
> + res.xattr_val_len = size;
> + res.xattr_val = kmalloc(size, GFP_KERNEL);
> + if (!res.xattr_val)
> + return -ENOMEM;
> +
> + dprintk("NFS call getxattr %s%s %zd\n", namespace, name, size);
> +
> + msg.rpc_proc = &server->client_xattr->cl_procinfo[XATTRPROC3_GETXATTR];
> + nfs_fattr_init(&fattr);
> + status = rpc_call_sync(server->client_xattr, &msg, 0);
> +
> + dprintk("NFS reply getxattr: status=%d len=%d\n",
> + status, res.xattr_val_len);
> +
> + switch (status) {
> + case 0:
> + status = nfs_refresh_inode(inode, &fattr);
> + break;
> + case -EPFNOSUPPORT:
> + case -EPROTONOSUPPORT:
> + dprintk("NFS_V3_XATTR extension not supported; disabling\n");
> + server->caps &= ~NFS_CAP_XATTR;
> + case -ENOTSUPP:
> + status = -EOPNOTSUPP;
> + default:
> + goto cleanup;
> + }
> +
> + status = res.xattr_val_len;
> + if (status <= size)
res.xattr_val_len was set to size, as was status, and none of the
3 has been changed, so here status can't be > size can it?
Was this just a safety to prevent overrun, or did you mean to
do some other check? (If a safety, then you'll still return
status > size, but with garbage in value, so i think you'd want
to also change status)
> + memcpy(value, res.xattr_val, status);
On Wed, 23 Jun 2010, Serge E. Hallyn wrote:
> > + status = res.xattr_val_len;
> > + if (status <= size)
>
> res.xattr_val_len was set to size, as was status, and none of the
> 3 has been changed, so here status can't be > size can it?
res is part of msg, and is updated by the RPC layer when decoding the
response (via xdr_decode_string_inplace()).
> Was this just a safety to prevent overrun, or did you mean to
> do some other check? (If a safety, then you'll still return
> status > size, but with garbage in value, so i think you'd want
> to also change status)
>
> > + memcpy(value, res.xattr_val, status);
>
Yes, the check stops us from copying more than the max. expected size to
'value'.
It looks like we do need to return -EINVAL if the check fails, and likely
the same with listxattr(). (Or is there a better error code for reporting
invalid messages from the peer?)
- James
--
James Morris
<[email protected]>
Quoting James Morris ([email protected]):
> On Wed, 23 Jun 2010, Serge E. Hallyn wrote:
>
> > > + status = res.xattr_val_len;
> > > + if (status <= size)
> >
> > res.xattr_val_len was set to size, as was status, and none of the
> > 3 has been changed, so here status can't be > size can it?
>
> res is part of msg, and is updated by the RPC layer when decoding the
> response (via xdr_decode_string_inplace()).
Ah, I see, it's passed in as a member of msg. Thanks.
> > Was this just a safety to prevent overrun, or did you mean to
> > do some other check? (If a safety, then you'll still return
> > status > size, but with garbage in value, so i think you'd want
> > to also change status)
> >
> > > + memcpy(value, res.xattr_val, status);
> >
>
> Yes, the check stops us from copying more than the max. expected size to
> 'value'.
>
> It looks like we do need to return -EINVAL if the check fails, and likely
> the same with listxattr(). (Or is there a better error code for reporting
> invalid messages from the peer?)
That'd be my pick.
-serge