| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sun, 12 May 2019 18:07:12 +0200
From: walter harms <wharms@....de>
To: David Howells <dhowells@...hat.com>
CC: colin.king@...onical.com, joe@...ches.com, jaltman@...istor.com,
linux-afs@...ts.infradead.org, kernel-janitors@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] afs: Fix afs_xattr_get_yfs() to not try freeing an
error value
Am 12.05.2019 09:45, schrieb David Howells:
> afs_xattr_get_yfs() tries to free yacl, which may hold an error value (say
> if yfs_fs_fetch_opaque_acl() failed and returned an error).
>
> Fix this by allocating yacl up front (since it's a fixed-length struct,
> unlike afs_acl) and passing it in to the RPC function. This also allows
> the flags to be placed in the object rather than passing them through to
> the RPC function.
>
> Fixes: ae46578b963f ("afs: Get YFS ACLs and information through xattrs")
> Signed-off-by: David Howells <dhowells@...hat.com>
> ---
>
> fs/afs/internal.h | 2 +
> fs/afs/xattr.c | 86 ++++++++++++++++++++++++++++------------------------
> fs/afs/yfsclient.c | 29 ++++--------------
> 3 files changed, 53 insertions(+), 64 deletions(-)
>
> diff --git a/fs/afs/internal.h b/fs/afs/internal.h
> index b3cd6e8ad59d..74ee0f8ef8dd 100644
> --- a/fs/afs/internal.h
> +++ b/fs/afs/internal.h
> @@ -1382,7 +1382,7 @@ struct yfs_acl {
> };
>
> extern void yfs_free_opaque_acl(struct yfs_acl *);
> -extern struct yfs_acl *yfs_fs_fetch_opaque_acl(struct afs_fs_cursor *, unsigned int);
> +extern struct yfs_acl *yfs_fs_fetch_opaque_acl(struct afs_fs_cursor *, struct yfs_acl *);
> extern int yfs_fs_store_opaque_acl2(struct afs_fs_cursor *, const struct afs_acl *);
>
> /*
> diff --git a/fs/afs/xattr.c b/fs/afs/xattr.c
> index b6c44e75b361..d12bcda911e1 100644
> --- a/fs/afs/xattr.c
> +++ b/fs/afs/xattr.c
> @@ -148,9 +148,8 @@ static int afs_xattr_get_yfs(const struct xattr_handler *handler,
> struct afs_vnode *vnode = AFS_FS_I(inode);
> struct yfs_acl *yacl = NULL;
> struct key *key;
> - unsigned int flags = 0;
> char buf[16], *data;
> - int which = 0, dsize, ret;
> + int which = 0, dsize, ret = -ENOMEM;
>
> if (strcmp(name, "acl") == 0)
> which = 0;
> @@ -163,20 +162,26 @@ static int afs_xattr_get_yfs(const struct xattr_handler *handler,
> else
> return -EOPNOTSUPP;
>
> + yacl = kzalloc(sizeof(struct yfs_acl), GFP_KERNEL);
> + if (!yacl)
> + goto error;
> +
> if (which == 0)
> - flags |= YFS_ACL_WANT_ACL;
> + yacl->flags |= YFS_ACL_WANT_ACL;
> else if (which == 3)
> - flags |= YFS_ACL_WANT_VOL_ACL;
> + yacl->flags |= YFS_ACL_WANT_VOL_ACL;
>
> key = afs_request_key(vnode->volume->cell);
> - if (IS_ERR(key))
> - return PTR_ERR(key);
> + if (IS_ERR(key)) {
> + ret = PTR_ERR(key);
> + goto error_yacl;
> + }
>
> ret = -ERESTARTSYS;
> if (afs_begin_vnode_operation(&fc, vnode, key)) {
> while (afs_select_fileserver(&fc)) {
> fc.cb_break = afs_calc_vnode_cb_break(vnode);
> - yacl = yfs_fs_fetch_opaque_acl(&fc, flags);
> + yfs_fs_fetch_opaque_acl(&fc, yacl);
> }
>
> afs_check_for_remote_deletion(&fc, fc.vnode);
> @@ -184,44 +189,45 @@ static int afs_xattr_get_yfs(const struct xattr_handler *handler,
> ret = afs_end_vnode_operation(&fc);
> }
>
> - if (ret == 0) {
> - switch (which) {
> - case 0:
> - data = yacl->acl->data;
> - dsize = yacl->acl->size;
> - break;
> - case 1:
> - data = buf;
> - dsize = snprintf(buf, sizeof(buf), "%u",
> - yacl->inherit_flag);
> - break;
> - case 2:
> - data = buf;
> - dsize = snprintf(buf, sizeof(buf), "%u",
> - yacl->num_cleaned);
> - break;
> - case 3:
> - data = yacl->vol_acl->data;
> - dsize = yacl->vol_acl->size;
> - break;
> - default:
> - ret = -EOPNOTSUPP;
> - goto out;
> - }
> + if (ret < 0)
> + goto error_key;
> +
> + switch (which) {
> + case 0:
> + data = yacl->acl->data;
> + dsize = yacl->acl->size;
> + break;
> + case 1:
> + data = buf;
> + dsize = snprintf(buf, sizeof(buf), "%u", yacl->inherit_flag);
> + break;
> + case 2:
> + data = buf;
> + dsize = snprintf(buf, sizeof(buf), "%u", yacl->num_cleaned);
> + break;
> + case 3:
> + data = yacl->vol_acl->data;
> + dsize = yacl->vol_acl->size;
> + break;
> + default:
> + ret = -EOPNOTSUPP;
> + goto error_key;
> + }
>
> - ret = dsize;
> - if (size > 0) {
> - if (dsize > size) {
> - ret = -ERANGE;
> - goto out;
> - }
> - memcpy(buffer, data, dsize);
> + ret = dsize;
> + if (size > 0) {
> + if (dsize > size) {
> + ret = -ERANGE;
> + goto error_key;
> }
> + memcpy(buffer, data, dsize);
> }
>
i am confused: if size is <= 0 then the error is in dsize ?
re,
wh
> -out:
> - yfs_free_opaque_acl(yacl);
> +error_key:
> key_put(key);
> +error_yacl:
> + yfs_free_opaque_acl(yacl);
> +error:
> return ret;
> }
>
> diff --git a/fs/afs/yfsclient.c b/fs/afs/yfsclient.c
> index 6cf7d161baa1..d3e9e3fe0b58 100644
> --- a/fs/afs/yfsclient.c
> +++ b/fs/afs/yfsclient.c
> @@ -2333,12 +2333,6 @@ void yfs_free_opaque_acl(struct yfs_acl *yacl)
> }
> }
>
> -static void yfs_destroy_fs_fetch_opaque_acl(struct afs_call *call)
> -{
> - yfs_free_opaque_acl(call->reply[0]);
> - afs_flat_call_destructor(call);
> -}
> -
> /*
> * YFS.FetchOpaqueACL operation type
> */
> @@ -2346,18 +2340,17 @@ static const struct afs_call_type yfs_RXYFSFetchOpaqueACL = {
> .name = "YFS.FetchOpaqueACL",
> .op = yfs_FS_FetchOpaqueACL,
> .deliver = yfs_deliver_fs_fetch_opaque_acl,
> - .destructor = yfs_destroy_fs_fetch_opaque_acl,
> + .destructor = afs_flat_call_destructor,
> };
>
> /*
> * Fetch the YFS advanced ACLs for a file.
> */
> struct yfs_acl *yfs_fs_fetch_opaque_acl(struct afs_fs_cursor *fc,
> - unsigned int flags)
> + struct yfs_acl *yacl)
> {
> struct afs_vnode *vnode = fc->vnode;
> struct afs_call *call;
> - struct yfs_acl *yacl;
> struct afs_net *net = afs_v2net(vnode);
> __be32 *bp;
>
> @@ -2370,19 +2363,15 @@ struct yfs_acl *yfs_fs_fetch_opaque_acl(struct afs_fs_cursor *fc,
> sizeof(__be32) * 2 +
> sizeof(struct yfs_xdr_YFSFetchStatus) +
> sizeof(struct yfs_xdr_YFSVolSync));
> - if (!call)
> - goto nomem;
> -
> - yacl = kzalloc(sizeof(struct yfs_acl), GFP_KERNEL);
> - if (!yacl)
> - goto nomem_call;
> + if (!call) {
> + fc->ac.error = -ENOMEM;
> + return ERR_PTR(-ENOMEM);
> + }
>
> - yacl->flags = flags;
> call->key = fc->key;
> call->reply[0] = yacl;
> call->reply[1] = vnode;
> call->reply[2] = NULL; /* volsync */
> - call->ret_reply0 = true;
>
> /* marshall the parameters */
> bp = call->request;
> @@ -2396,12 +2385,6 @@ struct yfs_acl *yfs_fs_fetch_opaque_acl(struct afs_fs_cursor *fc,
> trace_afs_make_fs_call(call, &vnode->fid);
> afs_make_call(&fc->ac, call, GFP_KERNEL);
> return (struct yfs_acl *)afs_wait_for_call_to_complete(call, &fc->ac);
> -
> -nomem_call:
> - afs_put_call(call);
> -nomem:
> - fc->ac.error = -ENOMEM;
> - return ERR_PTR(-ENOMEM);
> }
>
> /*
>
>
Powered by blists - more mailing lists