Re: [PATCH can-next v4 3/3] can: add can diag interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] can: diag: unaligned 64-bit J1939_NAME and cdiag_ino in
  sk_diag_fill()
--

commit 6641e4d72c858dd57e3276451bf5e3299a63d8d2
Author: Filippo Storniolo <fstornio@redhat.com>

can: add can diag interface

This commit adds a SOCK_DIAG netlink interface and UAPI definitions for
monitoring open AF_CAN sockets from userspace.

> diff --git a/net/can/diag.c b/net/can/diag.c
> new file mode 100644
> index 0000000000000..11281f75ecd52
> --- /dev/null
> +++ b/net/can/diag.c
[ ... ]
> +static int sk_diag_fill(struct sock *sk, struct sk_buff *skb,
> +			struct can_diag_req *req, /* will be used for filtering */
> +			struct user_namespace *user_ns,
> +			u32 portid, u32 seq, u32 flags, u64 sk_ino)
> +{
> +	struct sockaddr_can can_addr;
> +	struct can_diag_msg *rep;
> +	struct nlmsghdr *nlh;
> +	int err;
> +
> +	nlh = nlmsg_put(skb, portid, seq, SOCK_DIAG_BY_FAMILY, sizeof(*rep), flags);
> +	if (!nlh)
> +		return -EMSGSIZE;
> +
> +	rep = nlmsg_data(nlh);
> +	memset(rep, 0, sizeof(struct can_diag_msg));
> +
> +	rep->cdiag_family = AF_CAN;
> +	rep->cdiag_type = sk->sk_type;
> +	rep->cdiag_ino = sk_ino;
[ ... ]
> +	case CAN_J1939:
> +	{
> +		name_t	j1939_name;
> +		pgn_t	j1939_pgn;
> +		__u8	j1939_addr;
> +
> +		j1939_name = can_addr.can_addr.j1939.name;
> +		j1939_pgn = can_addr.can_addr.j1939.pgn;
> +		j1939_addr = can_addr.can_addr.j1939.addr;
> +
> +		err = nla_put(skb, CAN_DIAG_J1939_NAME, sizeof(j1939_name), &j1939_name);

[Severity: Medium]
Should CAN_DIAG_J1939_NAME be emitted with nla_put_u64_64bit() instead of
nla_put()?

struct nlmsghdr (16 bytes), struct can_diag_msg (32 bytes), and the optional
CAN_DIAG_UID attribute (8 bytes) leave the attribute stream 8-byte aligned.
Because struct nlattr is 4 bytes, nla_put() places the 8-byte j1939_name
payload at a 4-byte-aligned offset and leaves the finished netlink message
length at a multiple of 4 rather than 8 (after the 8-byte CAN_DIAG_J1939_PGN
and padded 8-byte CAN_DIAG_J1939_ADDR attributes).

When can_diag_dump() packs another socket message into the same skb after a
CAN_J1939 socket, nlmsg_put() aligns only to NLMSG_ALIGNTO (4 bytes), so the
next struct can_diag_msg starts at an unaligned offset and rep->cdiag_ino =
sk_ino performs an unaligned 64-bit write in the kernel and unaligned reads
in userspace.  Using nla_put_u64_64bit() with a pad attribute and marking
cdiag_ino as __aligned_u64 in struct can_diag_msg would keep both the
attribute and subsequent messages aligned before the UAPI is frozen.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-feat-can-diag-v4-0-c28842c74c95@redhat.com?part=3