diff mbox series

[19/32] afs: Use mem_to_flex_dup() with struct afs_acl

Message ID 20220504014440.3697851-20-keescook@chromium.org
State New
Headers show
Series Introduce flexible array struct memcpy() helpers | expand

Commit Message

Kees Cook May 4, 2022, 1:44 a.m. UTC 
As part of the work to perform bounds checking on all memcpy() uses,
replace the open-coded a deserialization of bytes out of memory into a
trailing flexible array by using a flex_array.h helper to perform the
allocation, bounds checking, and copying.

Cc: David Howells <dhowells@redhat.com>
Cc: Marc Dionne <marc.dionne@auristor.com>
Cc: linux-afs@lists.infradead.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/afs/internal.h | 4 ++--
 fs/afs/xattr.c    | 7 ++-----
 2 files changed, 4 insertions(+), 7 deletions(-)

Comments

David Howells May 12, 2022, 9:41 p.m. UTC | #1 
Kees Cook <keescook@chromium.org> wrote:

>  struct afs_acl {
> -	u32	size;
> -	u8	data[];
> +	DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(u32, size);
> +	DECLARE_FLEX_ARRAY_ELEMENTS(u8, data);
>  };

Oof...  That's really quite unpleasant syntax.  Is it not possible to have
mem_to_flex_dup() and friends work without that?  You are telling them the
fields they have to fill in.

> +	struct afs_acl *acl = NULL;
>  
> -	acl = kmalloc(sizeof(*acl) + size, GFP_KERNEL);
> -	if (!acl) {
> +	if (mem_to_flex_dup(&acl, buffer, size, GFP_KERNEL)) {

Please don't do that.  Either do:

	acl = mem_to_flex_dup(buffer, size, GFP_KERNEL);
	if (!acl)

or:

	acl = mem_to_flex_dup(buffer, size, GFP_KERNEL);
	if (IS_ERR(acl))

Please especially don't make it that an apparent 'true' return indicates an
error.  If you absolutely must return the acl pointer through the argument
list (presumably because it's actually a macro), make it return false on
failure:

	if (!mem_to_flex_dup(&acl, buffer, size, GFP_KERNEL))

or return and explicitly check for an error code:

	if (mem_to_flex_dup(&acl, buffer, size, GFP_KERNEL) < 0)

or:

	ret = mem_to_flex_dup(&acl, buffer, size, GFP_KERNEL);
	if (ret < 0)

(or use != 0 rather than < 0)

David
diff mbox series

Patch

diff --git a/fs/afs/internal.h b/fs/afs/internal.h
index 7a72e9c60423..83014d20b6b3 100644
--- a/fs/afs/internal.h
+++ b/fs/afs/internal.h
@@ -1125,8 +1125,8 @@  extern bool afs_fs_get_capabilities(struct afs_net *, struct afs_server *,
 extern void afs_fs_inline_bulk_status(struct afs_operation *);
 
 struct afs_acl {
-	u32	size;
-	u8	data[];
+	DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(u32, size);
+	DECLARE_FLEX_ARRAY_ELEMENTS(u8, data);
 };
 
 extern void afs_fs_fetch_acl(struct afs_operation *);
diff --git a/fs/afs/xattr.c b/fs/afs/xattr.c
index 7751b0b3f81d..77b3af283d49 100644
--- a/fs/afs/xattr.c
+++ b/fs/afs/xattr.c
@@ -73,16 +73,13 @@  static int afs_xattr_get_acl(const struct xattr_handler *handler,
 static bool afs_make_acl(struct afs_operation *op,
 			 const void *buffer, size_t size)
 {
-	struct afs_acl *acl;
+	struct afs_acl *acl = NULL;
 
-	acl = kmalloc(sizeof(*acl) + size, GFP_KERNEL);
-	if (!acl) {
+	if (mem_to_flex_dup(&acl, buffer, size, GFP_KERNEL)) {
 		afs_op_nomem(op);
 		return false;
 	}
 
-	acl->size = size;
-	memcpy(acl->data, buffer, size);
 	op->acl = acl;
 	return true;
 }