linux-kernel - Re: [PATCH] acl: Annotate struct posix_acl with __counted

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20240924093826.e2eh4ub7fw2zuo7r@quack3>
Date: Tue, 24 Sep 2024 11:38:26 +0200
From: Jan Kara <jack@...e.cz>
To: Thorsten Blum <thorsten.blum@...ux.dev>
Cc: Alexander Viro <viro@...iv.linux.org.uk>,
	Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
	Kees Cook <kees@...nel.org>,
	"Gustavo A. R. Silva" <gustavoars@...nel.org>,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-hardening@...r.kernel.org
Subject: Re: [PATCH] acl: Annotate struct posix_acl with __counted_by()

On Mon 23-09-24 23:38:05, Thorsten Blum wrote:
> Add the __counted_by compiler attribute to the flexible array member
> a_entries to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and
> CONFIG_FORTIFY_SOURCE.
> 
> Use struct_size() to calculate the number of bytes to allocate for new
> and cloned acls and remove the local size variables.
> 
> Change the posix_acl_alloc() function parameter count from int to
> unsigned int to match posix_acl's a_count data type. Add identifier
> names to the function definition to silence two checkpatch warnings.
> 
> Signed-off-by: Thorsten Blum <thorsten.blum@...ux.dev>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@...e.cz>

								Honza

> ---
>  fs/posix_acl.c            | 13 ++++++-------
>  include/linux/posix_acl.h |  4 ++--
>  2 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/posix_acl.c b/fs/posix_acl.c
> index 6c66a37522d0..4050942ab52f 100644
> --- a/fs/posix_acl.c
> +++ b/fs/posix_acl.c
> @@ -200,11 +200,11 @@ EXPORT_SYMBOL(posix_acl_init);
>   * Allocate a new ACL with the specified number of entries.
>   */
>  struct posix_acl *
> -posix_acl_alloc(int count, gfp_t flags)
> +posix_acl_alloc(unsigned int count, gfp_t flags)
>  {
> -	const size_t size = sizeof(struct posix_acl) +
> -	                    count * sizeof(struct posix_acl_entry);
> -	struct posix_acl *acl = kmalloc(size, flags);
> +	struct posix_acl *acl;
> +
> +	acl = kmalloc(struct_size(acl, a_entries, count), flags);
>  	if (acl)
>  		posix_acl_init(acl, count);
>  	return acl;
> @@ -220,9 +220,8 @@ posix_acl_clone(const struct posix_acl *acl, gfp_t flags)
>  	struct posix_acl *clone = NULL;
>  
>  	if (acl) {
> -		int size = sizeof(struct posix_acl) + acl->a_count *
> -		           sizeof(struct posix_acl_entry);
> -		clone = kmemdup(acl, size, flags);
> +		clone = kmemdup(acl, struct_size(acl, a_entries, acl->a_count),
> +				flags);
>  		if (clone)
>  			refcount_set(&clone->a_refcount, 1);
>  	}
> diff --git a/include/linux/posix_acl.h b/include/linux/posix_acl.h
> index 0e65b3d634d9..83b2c5fba1d9 100644
> --- a/include/linux/posix_acl.h
> +++ b/include/linux/posix_acl.h
> @@ -30,7 +30,7 @@ struct posix_acl {
>  	refcount_t		a_refcount;
>  	struct rcu_head		a_rcu;
>  	unsigned int		a_count;
> -	struct posix_acl_entry	a_entries[];
> +	struct posix_acl_entry	a_entries[] __counted_by(a_count);
>  };
>  
>  #define FOREACH_ACL_ENTRY(pa, acl, pe) \
> @@ -62,7 +62,7 @@ posix_acl_release(struct posix_acl *acl)
>  /* posix_acl.c */
>  
>  extern void posix_acl_init(struct posix_acl *, int);
> -extern struct posix_acl *posix_acl_alloc(int, gfp_t);
> +extern struct posix_acl *posix_acl_alloc(unsigned int count, gfp_t flags);
>  extern struct posix_acl *posix_acl_from_mode(umode_t, gfp_t);
>  extern int posix_acl_equiv_mode(const struct posix_acl *, umode_t *);
>  extern int __posix_acl_create(struct posix_acl **, gfp_t, umode_t *);
> -- 
> 2.46.1
> 
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR