Re: [net-next,v17,09/14] net: homa: create homa_outgoing.c

[Paolo Abeni <pabeni@redhat.com>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: homa: create homa_outgoing.c

This file does most of the work of transmitting outgoing messages.
It is also responsible for copying data from user space into skbs.

> diff --git a/net/homa/homa_impl.h b/net/homa/homa_impl.h
> --- a/net/homa/homa_impl.h
> +++ b/net/homa/homa_impl.h

[ ... ]

> diff --git a/net/homa/homa_outgoing.c b/net/homa/homa_outgoing.c
> --- /dev/null
> +++ b/net/homa/homa_outgoing.c

[ ... ]

> +/**
> + * homa_xmit_unknown() - Send an RPC_UNKNOWN packet to a peer.
> + * @skb:         Buffer containing an incoming packet; identifies the peer to
> + *               which the RPC_UNKNOWN packet should be sent.
> + * @hsk:         Socket that should be used to send the RPC_UNKNOWN packet.
> + */
> +void homa_xmit_unknown(struct sk_buff *skb, struct homa_sock *hsk)
> +{
> +	struct homa_common_hdr *h = (struct homa_common_hdr *)skb->data;
> +	struct in6_addr saddr = skb_canonical_ipv6_saddr(skb);
> +	struct homa_rpc_unknown_hdr unknown;
                                        ^^^^^^^^^^
Does this code send uninitialized kernel stack memory over the network?
The struct homa_rpc_unknown_hdr unknown is only partially initialized.

Four fields are set:

	unknown.common.sport = h->dport;
	unknown.common.dport = h->sport;
	unknown.common.type = RPC_UNKNOWN;
	unknown.common.sender_id = cpu_to_be64(homa_local_id(h->sender_id));

But the remaining fields of homa_common_hdr (sequence, ack[3], doff,
reserved1, window, checksum, reserved2) are left uninitialized. These total
15 bytes of data.

__homa_xmit_control() copies the entire struct via memcpy():

	h = skb_put(skb, length);
	memcpy(h, contents, length);

and the packet is then transmitted over the network via ip_queue_xmit() or
ip6_xmit().

This sends uninitialized kernel stack memory contents over the wire, which is
an information leak.


> +	struct homa_peer *peer;
> +
> +	unknown.common.sport = h->dport;
> +	unknown.common.dport = h->sport;
> +	unknown.common.type = RPC_UNKNOWN;
> +	unknown.common.sender_id = cpu_to_be64(homa_local_id(h->sender_id));
> +	peer = homa_peer_get(hsk, &saddr);
> +	if (!IS_ERR(peer))
> +		__homa_xmit_control(&unknown, sizeof(unknown), peer, hsk);
> +	homa_peer_release(peer);
          ^^^^^^^^^^^^^^^^^^^^

Can this dereference an error pointer?  When homa_peer_get() fails and
returns an ERR_PTR (for example, -ENOMEM from allocation failure or
rhashtable insert error), homa_peer_release(peer) is called unconditionally on
the error pointer.

homa_peer_release() calls refcount_dec_and_test(&peer->refs), which
dereferences the invalid ERR_PTR address.

The IS_ERR check on the previous line correctly guards __homa_xmit_control
but does not guard homa_peer_release.


> +}

[ ... ]