[PATCH 1/2] afs: Fix incorrect error handling in afs_xattr_get_acl()

[PATCH 1/2] afs: Fix incorrect error handling in afs_xattr_get_acl()

[David Howells]

Fix incorrect error handling in afs_xattr_get_acl() where there appears to
be a redundant assignment before return, but in fact the return should be a
goto to the error handling at the end of the function.

Fixes: 260f082bae6d ("afs: Get an AFS3 ACL as an xattr")
Addresses-Coverity: ("Unused Value")
Reported-by: Colin Ian King <colin.king@canonical.com>
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Joe Perches <joe@perches.com>
---

 fs/afs/xattr.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/fs/afs/xattr.c b/fs/afs/xattr.c
index c81f85003fc7..b6c44e75b361 100644
--- a/fs/afs/xattr.c
+++ b/fs/afs/xattr.c
@@ -71,11 +71,10 @@ static int afs_xattr_get_acl(const struct xattr_handler *handler,
 	if (ret == 0) {
 		ret = acl->size;
 		if (size > 0) {
-			ret = -ERANGE;
-			if (acl->size > size)
-				return -ERANGE;
-			memcpy(buffer, acl->data, acl->size);
-			ret = acl->size;
+			if (acl->size <= size)
+				memcpy(buffer, acl->data, acl->size);
+			else
+				ret = -ERANGE;
 		}
 		kfree(acl);
 	}

[PATCH 2/2] afs: Fix afs_xattr_get_yfs() to not try freeing an error value

[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@redhat.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);
 	}
 
-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);
 }
 
 /*

Re: [PATCH 2/2] afs: Fix afs_xattr_get_yfs() to not try freeing an error value

[walter harms]

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@redhat.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);
>  }
>  
>  /*
> 
> 

Re: [PATCH 2/2] afs: Fix afs_xattr_get_yfs() to not try freeing an error value

[David Howells]

walter harms <wharms@bfs.de> wrote:

> > +	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 ?

See this bit, before that hunk:

> +	if (ret < 0)
> +		goto error_key;

David

Re: [PATCH 2/2] afs: Fix afs_xattr_get_yfs() to not try freeing an error value

[walter harms]

Am 12.05.2019 20:10, schrieb David Howells:
> walter harms <wharms@bfs.de> wrote:
> 
>>> +	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 ?
> 
> See this bit, before that hunk:
> 
>> +	if (ret < 0)
>> +		goto error_key;
> 
> David
> 

Sorry, you misunderstood me, my fault, i did not see that size is unsigned.
NTL i do not think size=0 is useful.

You get size from outside, and if i follow the flow correct
the first use of it is to check size>0.
perhaps you can check size at start and simply return.
Now if size==0 it will return dsize and give the impression
that buffer is used (it is not).

while you are there:
  flags |= YFS_ACL_WANT_ACL is always flags = YFS_ACL_WANT_ACL;
since flags is 0 at this point.
IMHO that sould be moved to the strcmp() section.

hope that helps,

re,
 wh

Re: [PATCH 2/2] afs: Fix afs_xattr_get_yfs() to not try freeing an error value

[David Howells]

walter harms <wharms@bfs.de> wrote:

> Sorry, you misunderstood me, my fault, i did not see that size is unsigned.
> NTL i do not think size=0 is useful.

Allow me to quote from the getxattr manpage:

       If size is specified as zero, these calls return the  current  size  of
       the  named extended attribute (and leave value unchanged).  This can be
       used to determine the size of the buffer that should be supplied  in  a
       subsequent  call.   [...]

> while you are there:
>   flags |= YFS_ACL_WANT_ACL is always flags = YFS_ACL_WANT_ACL;
> since flags is 0 at this point.
> IMHO that sould be moved to the strcmp() section.

Why?  It makes the strcmp() section more complicated and means I now either
have to cache flags in a variable or do the allocation of yacl first.

David

Re: [PATCH 2/2] afs: Fix afs_xattr_get_yfs() to not try freeing an error value

[walter harms]

Am 12.05.2019 22:06, schrieb David Howells:
> walter harms <wharms@bfs.de> wrote:
> 
>> Sorry, you misunderstood me, my fault, i did not see that size is unsigned.
>> NTL i do not think size=0 is useful.
> 
> Allow me to quote from the getxattr manpage:
> 
>        If size is specified as zero, these calls return the  current  size  of
>        the  named extended attribute (and leave value unchanged).  This can be
>        used to determine the size of the buffer that should be supplied  in  a
>        subsequent  call.   [...]
>  
ok, sorry for the noise i did not know, for me that look unintended.



>> while you are there:
>>   flags |= YFS_ACL_WANT_ACL is always flags = YFS_ACL_WANT_ACL;
>> since flags is 0 at this point.
>> IMHO that sould be moved to the strcmp() section.
> 
> Why?  It makes the strcmp() section more complicated and means I now either
> have to cache flags in a variable or do the allocation of yacl first.
> 

no need to cache, the idea was only to make the correlation between the name
and flags more obvious. (no need to hurry, i just noticed it)

if (strcmp(name, "acl") == 0) {
		which = 0;
                flags = YFS_ACL_WANT_ACL;
	}
else if (strcmp(name, "acl_inherited") == 0) {
		which = 1;
                flags = 0;
        }
else if (strcmp(name, "acl_num_cleaned") == 0) {
		which = 2;
                flags = 0;
	}
else if (strcmp(name, "vol_acl") == 0) {
		which = 3;
                flags = YFS_ACL_WANT_VOL_ACL;
}
....

re,
 wh
> David
>