| Message ID | 161547182293.1868820.9860274141056722598.stgit@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | AFS metadata xattr fixes | expand |
On Thu, Mar 11, 2021 at 10:10 AM David Howells <dhowells@redhat.com> wrote: > > If someone attempts to access YFS-related xattrs (e.g. afs.yfs.acl) on a > file on a non-YFS AFS server (such as OpenAFS), then the kernel will jump > to a NULL function pointer because the afs_fetch_acl_operation descriptor > doesn't point to a function for issuing an operation on a non-YFS > server[1]. > > Fix this by making afs_wait_for_operation() check that the issue_afs_rpc > method is set before jumping to it and setting -ENOTSUPP if not. This fix > also covers other potential operations that also only exist on YFS servers. > > afs_xattr_get/set_yfs() then need to translate -ENOTSUPP to -ENODATA as the > former error is internal to the kernel. > > The bug shows up as an oops like the following: > > BUG: kernel NULL pointer dereference, address: 0000000000000000 > [...] > Code: Unable to access opcode bytes at RIP 0xffffffffffffffd6. > [...] > Call Trace: > afs_wait_for_operation+0x83/0x1b0 [kafs] > afs_xattr_get_yfs+0xe6/0x270 [kafs] > __vfs_getxattr+0x59/0x80 > vfs_getxattr+0x11c/0x140 > getxattr+0x181/0x250 > ? __check_object_size+0x13f/0x150 > ? __fput+0x16d/0x250 > __x64_sys_fgetxattr+0x64/0xb0 > do_syscall_64+0x49/0xc0 > entry_SYSCALL_64_after_hwframe+0x44/0xa9 > RIP: 0033:0x7fb120a9defe > > This was triggered with "cp -a" which attempts to copy xattrs, including > afs ones, but is easier to reproduce with getfattr, e.g.: > > getfattr -d -m ".*" /afs/openafs.org/ > > Fixes: e49c7b2f6de7 ("afs: Build an abstraction around an "operation" concept") > Reported-by: Gaja Sophie Peters <gaja.peters@math.uni-hamburg.de> > Signed-off-by: David Howells <dhowells@redhat.com> > Tested-by: Gaja Sophie Peters <gaja.peters@math.uni-hamburg.de> > cc: linux-afs@lists.infradead.org > Link: http://lists.infradead.org/pipermail/linux-afs/2021-March/003498.html [1] > --- > > fs/afs/fs_operation.c | 7 +++++-- > fs/afs/xattr.c | 8 +++++++- > 2 files changed, 12 insertions(+), 3 deletions(-) > > diff --git a/fs/afs/fs_operation.c b/fs/afs/fs_operation.c > index 97cab12b0a6c..71c58723763d 100644 > --- a/fs/afs/fs_operation.c > +++ b/fs/afs/fs_operation.c > @@ -181,10 +181,13 @@ void afs_wait_for_operation(struct afs_operation *op) > if (test_bit(AFS_SERVER_FL_IS_YFS, &op->server->flags) && > op->ops->issue_yfs_rpc) > op->ops->issue_yfs_rpc(op); > - else > + else if (op->ops->issue_afs_rpc) > op->ops->issue_afs_rpc(op); > + else > + op->ac.error = -ENOTSUPP; > > - op->error = afs_wait_for_call_to_complete(op->call, &op->ac); > + if (op->call) > + op->error = afs_wait_for_call_to_complete(op->call, &op->ac); > } > > switch (op->error) { > diff --git a/fs/afs/xattr.c b/fs/afs/xattr.c > index c629caae5002..4934e325a14a 100644 > --- a/fs/afs/xattr.c > +++ b/fs/afs/xattr.c > @@ -231,6 +231,8 @@ static int afs_xattr_get_yfs(const struct xattr_handler *handler, > else > ret = -ERANGE; > } > + } else if (ret == -ENOTSUPP) { > + ret = -ENODATA; > } > > error_yacl: > @@ -256,6 +258,7 @@ static int afs_xattr_set_yfs(const struct xattr_handler *handler, > { > struct afs_operation *op; > struct afs_vnode *vnode = AFS_FS_I(inode); > + int ret; > > if (flags == XATTR_CREATE || > strcmp(name, "acl") != 0) > @@ -270,7 +273,10 @@ static int afs_xattr_set_yfs(const struct xattr_handler *handler, > return afs_put_operation(op); > > op->ops = &yfs_store_opaque_acl2_operation; > - return afs_do_sync_operation(op); > + ret = afs_do_sync_operation(op); > + if (ret == -ENOTSUPP) > + ret = -ENODATA; > + return ret; > } > > static const struct xattr_handler afs_xattr_yfs_handler = { Reviewed-by: Marc Dionne <marc.dionne@auristor.com> Marc
diff --git a/fs/afs/fs_operation.c b/fs/afs/fs_operation.c index 97cab12b0a6c..71c58723763d 100644 --- a/fs/afs/fs_operation.c +++ b/fs/afs/fs_operation.c @@ -181,10 +181,13 @@ void afs_wait_for_operation(struct afs_operation *op) if (test_bit(AFS_SERVER_FL_IS_YFS, &op->server->flags) && op->ops->issue_yfs_rpc) op->ops->issue_yfs_rpc(op); - else + else if (op->ops->issue_afs_rpc) op->ops->issue_afs_rpc(op); + else + op->ac.error = -ENOTSUPP; - op->error = afs_wait_for_call_to_complete(op->call, &op->ac); + if (op->call) + op->error = afs_wait_for_call_to_complete(op->call, &op->ac); } switch (op->error) { diff --git a/fs/afs/xattr.c b/fs/afs/xattr.c index c629caae5002..4934e325a14a 100644 --- a/fs/afs/xattr.c +++ b/fs/afs/xattr.c @@ -231,6 +231,8 @@ static int afs_xattr_get_yfs(const struct xattr_handler *handler, else ret = -ERANGE; } + } else if (ret == -ENOTSUPP) { + ret = -ENODATA; } error_yacl: @@ -256,6 +258,7 @@ static int afs_xattr_set_yfs(const struct xattr_handler *handler, { struct afs_operation *op; struct afs_vnode *vnode = AFS_FS_I(inode); + int ret; if (flags == XATTR_CREATE || strcmp(name, "acl") != 0) @@ -270,7 +273,10 @@ static int afs_xattr_set_yfs(const struct xattr_handler *handler, return afs_put_operation(op); op->ops = &yfs_store_opaque_acl2_operation; - return afs_do_sync_operation(op); + ret = afs_do_sync_operation(op); + if (ret == -ENOTSUPP) + ret = -ENODATA; + return ret; } static const struct xattr_handler afs_xattr_yfs_handler = {
If someone attempts to access YFS-related xattrs (e.g. afs.yfs.acl) on a file on a non-YFS AFS server (such as OpenAFS), then the kernel will jump to a NULL function pointer because the afs_fetch_acl_operation descriptor doesn't point to a function for issuing an operation on a non-YFS server[1]. Fix this by making afs_wait_for_operation() check that the issue_afs_rpc method is set before jumping to it and setting -ENOTSUPP if not. This fix also covers other potential operations that also only exist on YFS servers. afs_xattr_get/set_yfs() then need to translate -ENOTSUPP to -ENODATA as the former error is internal to the kernel. The bug shows up as an oops like the following: BUG: kernel NULL pointer dereference, address: 0000000000000000 [...] Code: Unable to access opcode bytes at RIP 0xffffffffffffffd6. [...] Call Trace: afs_wait_for_operation+0x83/0x1b0 [kafs] afs_xattr_get_yfs+0xe6/0x270 [kafs] __vfs_getxattr+0x59/0x80 vfs_getxattr+0x11c/0x140 getxattr+0x181/0x250 ? __check_object_size+0x13f/0x150 ? __fput+0x16d/0x250 __x64_sys_fgetxattr+0x64/0xb0 do_syscall_64+0x49/0xc0 entry_SYSCALL_64_after_hwframe+0x44/0xa9 RIP: 0033:0x7fb120a9defe This was triggered with "cp -a" which attempts to copy xattrs, including afs ones, but is easier to reproduce with getfattr, e.g.: getfattr -d -m ".*" /afs/openafs.org/ Fixes: e49c7b2f6de7 ("afs: Build an abstraction around an "operation" concept") Reported-by: Gaja Sophie Peters <gaja.peters@math.uni-hamburg.de> Signed-off-by: David Howells <dhowells@redhat.com> Tested-by: Gaja Sophie Peters <gaja.peters@math.uni-hamburg.de> cc: linux-afs@lists.infradead.org Link: http://lists.infradead.org/pipermail/linux-afs/2021-March/003498.html [1] --- fs/afs/fs_operation.c | 7 +++++-- fs/afs/xattr.c | 8 +++++++- 2 files changed, 12 insertions(+), 3 deletions(-)