| Message ID | 20220504014440.3697851-20-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | Introduce flexible array struct memcpy() helpers | expand |
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
On Thu, May 12, 2022 at 10:41:05PM +0100, David Howells wrote: > > 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. Other threads discussed this too. I'm hoping to have something more flexible (pardon the pun) in v2. > [...] > or: > > ret = mem_to_flex_dup(&acl, buffer, size, GFP_KERNEL); > if (ret < 0) > > (or use != 0 rather than < 0) Sure, I can make the tests more explicit. The kerndoc, etc all shows it's using < 0 for errors.
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; }
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(-)