Re: [PATCH v6 bpf-next 1/4] security: introduce struct lsm_xattrs

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Latent buffer overflow: `struct lsm_xattrs` lacks a capacity field, and `lsm_get_xattr_slot()` increments `xattr_count` without bounds checking.
--

--- Patch [1]: [PATCH v6 bpf-next 1/4] security: introduce struct lsm_xattrs ---
commit 4d965e040bf3aa7aac8ca18905c2c3f09a037f96
Author: David Windsor <[email protected]>

security: introduce struct lsm_xattrs

This patch introduces struct lsm_xattrs to consolidate the LSM xattr array
and its count into a single context object. This preparation allows better
compatibility with the BPF verifier for a planned kfunc that lets BPF LSM
programs atomically label new inodes.

> diff --git a/include/linux/security.h b/include/linux/security.h
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
[ ... ]
> @@ -68,6 +68,11 @@ struct watch;
>  struct watch_notification;
>  struct lsm_ctx;
>  
> +struct lsm_xattrs {
> +	struct xattr *xattrs;
> +	unsigned int xattr_count;
> +};
> +

[Severity: Medium]
Should this struct also track the maximum capacity of the array?

Since this object is being introduced to allow dynamically loaded BPF LSM
programs to label new inodes, a BPF program calling the planned kfunc
multiple times could potentially exceed the allocated capacity. Tracking
the max capacity would allow for bounds checking.

>  /* Default (no) options for the capable function */
>  #define CAP_OPT_NONE 0x0
[ ... ]
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
[ ... ]
> -static inline struct xattr *lsm_get_xattr_slot(struct xattr *xattrs,
> -					       int *xattr_count)
> +static inline struct xattr *lsm_get_xattr_slot(struct lsm_xattrs *ctx)
>  {
> -	if (unlikely(!xattrs))
> +	if (unlikely(!ctx || !ctx->xattrs))
>  		return NULL;
> -	return &xattrs[(*xattr_count)++];
> +	return &ctx->xattrs[ctx->xattr_count++];
>  }

[Severity: Medium]
Does this increment need a bounds check against the array capacity?

If a dynamic BPF LSM program requests more slots than were allocated in
security_inode_init_security() (via blob_sizes.lbs_xattr_count + 1), this
would result in an out-of-bounds write to the array.

This could subsequently lead to memory corruption during the cleanup loop
that calls kfree() on the array elements.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1