Re: [PATCH net v3 3/6] amt: reject an IPv6 address passed in an IPv4 address attribute

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix does not close the bug class its commit message
  describes.

--

amt: reject an IPv6 address passed in an IPv4 address attribute

This patch adds amt_ip6_in_ip4_attr() and calls it from amt_validate().
amt_validate() then rejects an IFLA_AMT_LOCAL_IP or IFLA_AMT_DISCOVERY_IP
attribute whose payload is exactly 16 bytes. The aim is to stop
nla_get_in_addr() from silently truncating an IPv6 literal to an IPv4
address.

> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index c3716bad251f6..550b0a5e73766 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c
> @@ -3194,6 +3194,15 @@ static const struct nla_policy amt_policy[IFLA_AMT_MAX + 1] = {
>  	[IFLA_AMT_MAX_TUNNELS]	= { .type = NLA_U32 },
>  };
>  
> +static bool amt_ip6_in_ip4_attr(const struct nlattr *attr,
> +				struct netlink_ext_ack *extack)
> +{
> +	if (!attr || nla_len(attr) != sizeof(struct in6_addr))
> +		return false;

[Severity: High]
Can other oversized lengths still get through and be truncated?

The commit message names these policy entries as the root cause, but the
patch does not change them:

drivers/net/amt.c:amt_policy[] {
    ...
	[IFLA_AMT_LOCAL_IP]	= { .len = sizeof_field(struct iphdr, daddr) },
	...
	[IFLA_AMT_DISCOVERY_IP]	= { .len = sizeof_field(struct iphdr, daddr) },
    ...
}

These entries are NLA_UNSPEC, so validate_nla() in lib/nlattr.c treats .len
only as a minimum length:

	if (attrlen < minlen)
		goto out_err;

Some lengths still pass the policy: 5 to 15 bytes, and 17 bytes or more.
amt_ip6_in_ip4_attr() returns false for these because the length is not
exactly 16.

amt_newlink() then truncates the value in the same way:

	amt->local_ip = nla_get_in_addr(data[IFLA_AMT_LOCAL_IP]);
	...
	amt->discovery_ip = nla_get_in_addr(data[IFLA_AMT_DISCOVERY_IP]);

The device is then created with an address that was never requested.

Would it be better to change these two policy entries to
NLA_POLICY_EXACT_LEN(sizeof(__be32)) or NLA_BE32? That would reject every
wrong length with a standard bad-attribute extack, and the open-coded
helper would no longer be needed. amt_get_size() and amt_fill_info()
already emit exactly sizeof(__be32) for these attributes.

The problem is still there at the end of the series. The last patch, "amt:
do not create tunnel state for unauthenticated Requests", leaves both
amt_policy and amt_ip6_in_ip4_attr() unchanged.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012125943.558629-1-omar%40blockcast.net