diff mbox series

[1/2] afs: Fix accessing YFS xattrs on a non-YFS server

Message ID 161547182293.1868820.9860274141056722598.stgit@warthog.procyon.org.uk (mailing list archive)
State New
Headers show
Series AFS metadata xattr fixes | expand

Commit Message

David Howells March 11, 2021, 2:10 p.m. UTC
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(-)

Comments

Marc Dionne March 11, 2021, 6:26 p.m. UTC | #1
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 mbox series

Patch

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 = {