* [PATCH 1/2] afs: Fix accessing YFS xattrs on a non-YFS server
2021-03-11 14:10 [PATCH 0/2] AFS metadata xattr fixes David Howells
@ 2021-03-11 14:10 ` David Howells
2021-03-11 18:26 ` Marc Dionne
2021-03-11 14:10 ` [PATCH 2/2] afs: Fix afs_listxattr() to not list afs ACL special xattrs David Howells
2021-03-11 14:54 ` [PATCH 0/2] AFS metadata xattr fixes Jeffrey E Altman
2 siblings, 1 reply; 6+ messages in thread
From: David Howells @ 2021-03-11 14:10 UTC (permalink / raw)
To: linux-afs
Cc: Gaja Sophie Peters, Gaja Sophie Peters, dhowells,
Gaja Sophie Peters, linux-fsdevel, linux-kernel
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 = {
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] afs: Fix accessing YFS xattrs on a non-YFS server
2021-03-11 14:10 ` [PATCH 1/2] afs: Fix accessing YFS xattrs on a non-YFS server David Howells
@ 2021-03-11 18:26 ` Marc Dionne
0 siblings, 0 replies; 6+ messages in thread
From: Marc Dionne @ 2021-03-11 18:26 UTC (permalink / raw)
To: David Howells
Cc: linux-afs, Gaja Sophie Peters, linux-fsdevel,
Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] afs: Fix afs_listxattr() to not list afs ACL special xattrs
2021-03-11 14:10 [PATCH 0/2] AFS metadata xattr fixes David Howells
2021-03-11 14:10 ` [PATCH 1/2] afs: Fix accessing YFS xattrs on a non-YFS server David Howells
@ 2021-03-11 14:10 ` David Howells
2021-03-11 18:59 ` Marc Dionne
2021-03-11 14:54 ` [PATCH 0/2] AFS metadata xattr fixes Jeffrey E Altman
2 siblings, 1 reply; 6+ messages in thread
From: David Howells @ 2021-03-11 14:10 UTC (permalink / raw)
To: linux-afs
Cc: Gaja Sophie Peters, Gaja Sophie Peters, dhowells,
Gaja Sophie Peters, linux-fsdevel, linux-kernel
afs_listxattr() lists all the available special afs xattrs (i.e. those in
the "afs.*" space), no matter what type of server we're dealing with. But
OpenAFS servers, for example, cannot deal with some of the extra-capable
attributes that AuriStor (YFS) servers provide. Unfortunately, the
presence of the afs.yfs.* attributes causes errors[1] for anything that
tries to read them if the server is of the wrong type.
Fix afs_listxattr() so that it doesn't list any of the AFS ACL xattrs. It
does mean, however, that getfattr won't list them, though they can still be
accessed with getxattr() and setxattr().
This can be tested with something like:
getfattr -d -m ".*" /afs/example.com/path/to/file
With this change, none of the afs.* ACL attributes should be visible.
Fixes: ae46578b963f ("afs: Get YFS ACLs and information through xattrs")
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/003502.html [1]
---
fs/afs/xattr.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/fs/afs/xattr.c b/fs/afs/xattr.c
index 4934e325a14a..81a6aec764cc 100644
--- a/fs/afs/xattr.c
+++ b/fs/afs/xattr.c
@@ -12,14 +12,9 @@
#include "internal.h"
static const char afs_xattr_list[] =
- "afs.acl\0"
"afs.cell\0"
"afs.fid\0"
- "afs.volume\0"
- "afs.yfs.acl\0"
- "afs.yfs.acl_inherited\0"
- "afs.yfs.acl_num_cleaned\0"
- "afs.yfs.vol_acl";
+ "afs.volume";
/*
* Retrieve a list of the supported xattrs.
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] afs: Fix afs_listxattr() to not list afs ACL special xattrs
2021-03-11 14:10 ` [PATCH 2/2] afs: Fix afs_listxattr() to not list afs ACL special xattrs David Howells
@ 2021-03-11 18:59 ` Marc Dionne
0 siblings, 0 replies; 6+ messages in thread
From: Marc Dionne @ 2021-03-11 18:59 UTC (permalink / raw)
To: David Howells
Cc: linux-afs, Gaja Sophie Peters, linux-fsdevel,
Linux Kernel Mailing List
On Thu, Mar 11, 2021 at 10:10 AM David Howells <dhowells@redhat.com> wrote:
>
> afs_listxattr() lists all the available special afs xattrs (i.e. those in
> the "afs.*" space), no matter what type of server we're dealing with. But
> OpenAFS servers, for example, cannot deal with some of the extra-capable
> attributes that AuriStor (YFS) servers provide. Unfortunately, the
> presence of the afs.yfs.* attributes causes errors[1] for anything that
> tries to read them if the server is of the wrong type.
>
> Fix afs_listxattr() so that it doesn't list any of the AFS ACL xattrs. It
> does mean, however, that getfattr won't list them, though they can still be
> accessed with getxattr() and setxattr().
>
> This can be tested with something like:
>
> getfattr -d -m ".*" /afs/example.com/path/to/file
>
> With this change, none of the afs.* ACL attributes should be visible.
>
> Fixes: ae46578b963f ("afs: Get YFS ACLs and information through xattrs")
> 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/003502.html [1]
> ---
>
> fs/afs/xattr.c | 7 +------
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/fs/afs/xattr.c b/fs/afs/xattr.c
> index 4934e325a14a..81a6aec764cc 100644
> --- a/fs/afs/xattr.c
> +++ b/fs/afs/xattr.c
> @@ -12,14 +12,9 @@
> #include "internal.h"
>
> static const char afs_xattr_list[] =
> - "afs.acl\0"
> "afs.cell\0"
> "afs.fid\0"
> - "afs.volume\0"
> - "afs.yfs.acl\0"
> - "afs.yfs.acl_inherited\0"
> - "afs.yfs.acl_num_cleaned\0"
> - "afs.yfs.vol_acl";
> + "afs.volume";
>
> /*
> * Retrieve a list of the supported xattrs.
As Jeffrey suggested, you may want to consider not exposing any of
them, but this seems fine to me.
Besides the reasons you mention, acls getting copied blindly is not a
good idea. The source and target may be in different cells where
users and groups mean different things. For files, it may switch them
from inheriting permissions from their parent to having their own ACL,
etc.
Reviewed-by: Marc Dionne <marc.dionne@auristor.com>
Marc
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] AFS metadata xattr fixes
2021-03-11 14:10 [PATCH 0/2] AFS metadata xattr fixes David Howells
2021-03-11 14:10 ` [PATCH 1/2] afs: Fix accessing YFS xattrs on a non-YFS server David Howells
2021-03-11 14:10 ` [PATCH 2/2] afs: Fix afs_listxattr() to not list afs ACL special xattrs David Howells
@ 2021-03-11 14:54 ` Jeffrey E Altman
2 siblings, 0 replies; 6+ messages in thread
From: Jeffrey E Altman @ 2021-03-11 14:54 UTC (permalink / raw)
To: David Howells (dhowells@redhat.com), linux-afs
Cc: Gaja Sophie Peters, linux-fsdevel, linux-kernel
[-- Attachment #1.1: Type: text/plain, Size: 826 bytes --]
On 3/11/2021 9:10 AM, David Howells (dhowells@redhat.com) wrote:
> I wonder if it's better to just hide all the afs.* xattrs from listxattr().
> It would probably be even better to not use xattrs for this, but I'm not
> sure what I would use instead.
I believe that all of the "afs.*" xattrs should be hidden from
listxattr(). Any "afs.*" xattr that is read from an afs inode can be
copied to another filesystem and stored. Attempts to set these values
in an afs volume will fail.
The use of xattrs is intended to be an alternative to the IBM/OpenAFS
pioctls which are accessed by processes such as "fs", "aklog", "tokens",
etc which would know the names of the xattrs and how to use them. Such
tools would not require listxattr() to find them.
Thanks for the consideration.
Jeffrey Altman
[-- Attachment #1.2: jaltman.vcf --]
[-- Type: text/x-vcard, Size: 283 bytes --]
begin:vcard
fn:Jeffrey Altman
n:Altman;Jeffrey
org:AuriStor, Inc.
adr:;;255 W 94TH ST STE 6B;New York;NY;10025-6985;United States
email;internet:jaltman@auristor.com
title:CEO
tel;work:+1-212-769-9018
url:https://www.linkedin.com/in/jeffreyaltman/
version:2.1
end:vcard
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4033 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread