Re: [PATCH net v5 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 more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message for "amt: reject a gateway Query or Data from
  0.0.0.0 before a relay is learned" gets part of the root cause wrong…

--

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

This patch adds a !remote_ip guard to the Membership Query and Multicast
Data source checks in amt_rcv(), so a gateway with no learned relay
rejects packets from 0.0.0.0. It also rechecks the packet source against
the current remote_ip in amt_membership_query_handler(), which drops
Queries that were deferred across a relay address change.

> A gateway's amt_rcv() admits a Membership Query or Multicast Data message
> when "saddr != remote_ip" is false. remote_ip is 0 before discovery has
> learned a relay (and is reset to 0 by amt_dev_open() and when discovery
> gives up), so a packet whose source is 0.0.0.0 would otherwise pass the
> equality check and could spoof a Query or inject multicast data.

[Severity: Low]
The code change looks correct. Some parts of this description don't
match what drivers/net/amt.c does, though.

Is "when discovery gives up" accurate? amt_event_send_discovery() never
clears remote_ip. It only randomizes amt->nonce:

drivers/net/amt.c:amt_event_send_discovery() {
	if (amt->status > AMT_STATUS_SENT_DISCOVERY)
		goto out;
	get_random_bytes(&amt->nonce, sizeof(__be32));
    ...
}

The reset happens in the Request phase, in amt_event_send_request(),
once req_cnt exceeds AMT_MAX_REQ_COUNT:

		WRITE_ONCE(amt->remote_ip, 0);
		amt_update_gw_status(amt, AMT_STATUS_INIT, false);
		amt->req_cnt = 0;
		amt->nonce = 0;

amt_dev_stop() and amt_newlink() also clear remote_ip. The message does
not mention either of them.

Are the Query spoof and the Data injection really two separate
exposures? amt_multicast_data_handler() already drops every packet
unless the gateway is in AMT_STATUS_SENT_UPDATE:

	if (READ_ONCE(amt->status) != AMT_STATUS_SENT_UPDATE)
		return true;

With remote_ip == 0, the gateway can only be in that state in two cases.
The first is after a spoofed Query has moved status to RECEIVED_QUERY
and amt_dev_xmit()->amt_send_membership_update() has set SENT_UPDATE.
The second is the short gap between WRITE_ONCE(amt->remote_ip, 0) and
the status reset.

So the Data hunk looks like defense in depth rather than a separate
hole. Could the message say so?

The message also doesn't mention the condition that makes the Query
spoof work. amt_membership_query_handler() also checks the nonce:

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

amt->nonce is zeroed along with remote_ip in amt_dev_open() and in the
request give-up path above. A Query from 0.0.0.0 with nonce 0 would
therefore pass every check until the next discovery event randomizes
the nonce. Should the changelog state this?

The effect of a spoofed Query also seems larger than "could spoof a
Query". Besides injecting a forged IGMP/MLD query, the handler changes
gateway state:

		WRITE_ONCE(amt->ready4, true);
		amt->mac = response_mac;
		amt->req_cnt = 0;
		amt->qi = ihv3->qqic;

It also moves status from INIT to RECEIVED_QUERY. After that,
amt_event_send_discovery() bails out on the status check shown above.
amt_advertisement_handler() then rejects every Advertisement:

	if (amt->status != AMT_STATUS_SENT_DISCOVERY ||
	    amt->nonce != amta->nonce)
		return true;

Relay discovery stays blocked until the Request retry limit puts the
device back into INIT with nonce 0. At that point the spoof can be sent
again.

Would it be worth describing the nonce 0 condition and the blocked
discovery in the changelog, so stable backporters can judge the impact?

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