Re: [PATCH 1/3] net: netrom: fix integer overflow in nr_queue_rx_frame()

[Simon Horman]

From: 'Simon Horman' <horms@xxxxxxxxxx>

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

Simon says: This review combines a review of the patch with a report
of an existing problem. And you will see a similar pattern in
the AI generated reviews I am forwarding for the other patches in this
series.

I would suggest not growing the scope of this series too much.
But rather noting (other) existing problems as areas for further work.

Frankly, I suspect fixing all such problems is a significant body of work.

---
    net: netrom: fix integer overflow in nr_queue_rx_frame()

This commit addresses an integer overflow in nr_queue_rx_frame() where
accumulating fragment lengths could wrap around a 16-bit integer. It attempts
to fix this by checking if adding the incoming skb length to the accumulated
fraglen exceeds USHRT_MAX, purging the queue and returning an error if so.

> diff --git a/net/netrom/nr_in.c b/net/netrom/nr_in.c
> index 97944db6b5ac6..0b7cdb99ae501 100644
> --- a/net/netrom/nr_in.c
> +++ b/net/netrom/nr_in.c
> @@ -36,12 +36,22 @@ static int nr_queue_rx_frame(struct sock *sk, struct sk_buff *skb, int more)
>  	nr_start_idletimer(sk);
>  
>  	if (more) {
> +		if ((unsigned int)nr->fraglen + skb->len > USHRT_MAX) {
> +			skb_queue_purge(&nr->frag_queue);
> +			nr->fraglen = 0;
> +			return 1;
> +		}

If we return 1 here, does the caller (nr_state3_machine) treat it as a
transient busy condition?

If the caller retries the skb later, the skb will have already been modified
by the unconditional skb_pull() at the start of nr_queue_rx_frame(). Does
this mean the next time it is processed, it will read from the wrong offset
and potentially process attacker-controlled payload?

Furthermore, since nr->fraglen is reset to 0, wouldn't the oversized fragment
now pass the USHRT_MAX check on retry, bypassing the overflow mitigation
entirely?

When we purge the fragment queue here, are we also silently dropping
fragments that the protocol stack has already acknowledged to the sender?

Since nr_queue_rx_frame() previously returned 0 for those earlier fragments,
the expected sequence number has already advanced. If the sender never
retransmits them and fraglen is reset, will the receiver accept the next
fragment as the start of a new sequence, leading to a truncated message being
delivered to the application?

[ ... ]

While reviewing this patch, I also noticed a few pre-existing issues in
nr_queue_rx_frame() that might be worth addressing.

Looking at the start of the function:

static int nr_queue_rx_frame(struct sock *sk, struct sk_buff *skb, int more)
{
	struct sk_buff *skbo, *skbn = skb;
	struct nr_sock *nr = nr_sk(sk);

	skb_pull(skb, NR_NETWORK_LEN + NR_TRANSPORT_LEN);

Is it possible for skb_pull() to be called on a malformed packet where the
total length is sufficient but the linear region is too small? If so, would
this trigger a BUG_ON() panic inside __skb_pull()? Should there be a length
validation check using pskb_may_pull() beforehand?

Further down, during fragment reassembly:

	while ((skbo = skb_dequeue(&nr->frag_queue)) != NULL) {
		skb_copy_from_linear_data(skbo,
					  skb_put(skbn, skbo->len),
					  skbo->len);
		kfree_skb(skbo);
	}

If skbo is a non-linear (paged) skb, skbo->len includes the paged data length.
Since skb_copy_from_linear_data() acts as a direct memory copy, does this
result in reading past the end of the linear allocation (skb_headlen()) into
adjacent kernel memory?

Finally, looking at the error paths:

	if ((skbn = alloc_skb(nr->fraglen, GFP_ATOMIC)) == NULL)
		return 1;
	...
	return sock_queue_rcv_skb(sk, skbn);

If sock_queue_rcv_skb() fails, it returns a non-zero error which propagates
to the caller. Does the caller then try to re-enqueue the original fragment
pointer into a retry queue?

Since the original fragments were already dequeued from frag_queue and freed
via kfree_skb() in the loop above, could enqueuing the freed skb lead to a
use-after-free?

Additionally, if alloc_skb() fails and returns 1, does the caller enqueue the
original skb while it is still linked in frag_queue, potentially corrupting
the doubly-linked list?