Re: [RFC PATCH v2 05/41] af_unix: keep sender ids in skb->cb only when they are classic ids

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] af_unix: unix_skb_parms can exceed skb->cb size under CONFIG_RANDSTRUCT
--

commit 9d78c8ae60dfc024139d5c6d850877e3acd5d5ea
Author: Jan Koester <jan.koester@tuxist.de>

af_unix: keep sender ids in skb->cb only when they are classic ids

This commit keeps classic 32-bit sender uid and gid values inline in
unix_skb_parms while dynamically allocating a struct unix_skb_ids for
non-legacy ids so that unix_skb_parms continues to fit within the
48-byte skb->cb buffer.

> diff --git a/net/unix/af_unix.h b/net/unix/af_unix.h
> index 8119dbeef3a3c..e861a430cf37e 100644
> --- a/net/unix/af_unix.h
> +++ b/net/unix/af_unix.h
> @@ -10,10 +10,17 @@
>  
>  struct sock *unix_peer_get(struct sock *sk);
>  
> -struct unix_skb_parms {
> -	struct pid		*pid;		/* skb credentials	*/
> +/* The sender's ids, when either is a UUID or GUID */
> +struct unix_skb_ids {
>  	kuid_t			uid;
>  	kgid_t			gid;
> +};
> +
> +struct unix_skb_parms {
> +	struct pid		*pid;		/* skb credentials	*/
> +	struct unix_skb_ids	*ids;		/* uid and gid, or NULL	*/
> +	uid_t			uid;		/* for ids NULL: legacy	*/
> +	gid_t			gid;		/* uid and gid		*/
>  	struct scm_fp_list	*fp;		/* Passed files		*/
>  #ifdef CONFIG_SECURITY_NETWORK
>  	u32			secid;		/* Security ID		*/

[Severity: Low]
Can this cause a build regression on 64-bit kernels when CONFIG_RANDSTRUCT
and CONFIG_SECURITY_NETWORK are enabled?

In net/unix/af_unix.h, struct unix_skb_parms is marked with
__randomize_layout:

struct unix_skb_parms {
	struct pid		*pid;		/* skb credentials	*/
	struct unix_skb_ids	*ids;		/* uid and gid, or NULL	*/
	uid_t			uid;		/* for ids NULL: legacy	*/
	gid_t			gid;		/* uid and gid		*/
	struct scm_fp_list	*fp;		/* Passed files		*/
#ifdef CONFIG_SECURITY_NETWORK
	u32			secid;		/* Security ID		*/
#endif
	u32			consumed;
} __randomize_layout;

Adding the ids pointer while keeping uid and gid as separate 4-byte
members gives struct unix_skb_parms three 8-byte pointers (pid, ids, fp)
and four 4-byte integers (uid, gid, secid, consumed) on 64-bit builds
with CONFIG_SECURITY_NETWORK=y.

When a randstruct seed shuffles these seven members into an alternating
order of 4-byte integers and 8-byte pointers (u32, ptr, u32, ptr, u32,
ptr, u32), each 8-byte pointer is preceded by 4 bytes of alignment
padding and the trailing 4-byte integer is followed by 4 bytes of tail
padding.

That pads sizeof(struct unix_skb_parms) to 56 bytes, exceeding the
48-byte skb->cb buffer and triggering the compile-time assertion in
net/unix/af_unix.c:af_unix_init():

static int __init af_unix_init(void)
{
	int i, rc = -1;

	BUILD_BUG_ON(sizeof(struct unix_skb_parms) > sizeof_field(struct sk_buff, cb));

Would placing ids and the legacy uid/gid pair in a union (since uid and
gid are only used when ids is NULL), or grouping uid and gid into an
unnamed struct so they are randomized as a single 8-byte unit, avoid this
padding overhead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792552161.git.jan.koester@tuxist.de?part=5