Re: [PATCH net v4 2/5] amt: reject a gateway Query or Data from 0.0.0.0 before a relay is learned

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · 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).

- [Medium] The commit message does not mention one of the three hunks,
  and that hunk seems to belong to a later patch in the series.

--

amt: reject a gateway Query or Data from 0.0.0.0 before a relay is learned

This commit adds a !remote_ip guard to the Membership Query and Multicast
Data source checks in amt_rcv(). With the guard, a gateway that has not
learned a relay no longer accepts these messages from 0.0.0.0. It also
adds a source re-check in amt_membership_query_handler().

> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index b06916c7d57d5..2e0d3aae3759b 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c
> @@ -2413,6 +2413,10 @@ static bool amt_membership_query_handler(struct amt_dev *amt,
>  	if (amtmq->reserved || amtmq->version)
>  		return true;
>  
> +	/* A deferred query may outlive a relay address change. */
> +	if (ip_hdr(skb)->saddr != READ_ONCE(amt->remote_ip))
> +		return true;
> +

[Severity: Medium]
The commit message doesn't seem to describe this hunk. It only covers the
remote_ip == 0 case in amt_rcv():

  "Reject both when no relay is learned: a 0 remote_ip must match
  nothing, so guard the check with !remote_ip."

This check covers a different case. amt_rcv() checks the Query against
remote_ip when the packet arrives, and the handler runs later from the
event worker:

amt_rcv()
    amt_queue_event(amt, AMT_EVENT_RECEIVE, skb)
...
amt_event_work()
    amt_gw_rcv()
        amt_membership_query_handler()

In that window, amt_advertisement_handler() can change remote_ip. It can
also be reset to 0 by amt_event_send_request(), amt_dev_open() or
amt_dev_stop(). With this hunk, a Query that passed the check when it was
queued is dropped if the relay address changed before the worker ran.

The later patch in the series, "amt: harden relay authentication and
teardown", says in its commit message:

  "A deferred gateway Query is accepted only if its relay address still
  matches the current relay"

However, that patch's diff doesn't touch amt_membership_query_handler(),
and it carries a different Fixes: tag (cbc21dc1cfe9).

Did this hunk end up in the wrong patch? This commit has
Fixes: b9022b53adad and is likely to be picked for stable. Could the hunk
move to the patch that describes it, or could this commit message mention
it?

>  	if (amtmq->nonce != amt->nonce)
>  		return true;
>  

[ ... ]

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