Re: [PATCH 3/3] net: rose: fix out-of-bounds read in rose_parse_ccitt()

From: 'Simon Horman' <[email protected]>

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
---
net: rose: fix out-of-bounds read in rose_parse_ccitt()

This commit adds bounds checking to rose_parse_ccitt() for 0xC0-class
facilities to prevent an out-of-bounds read when parsing call request
frames. It ensures the remaining buffer contains enough bytes before
accessing the facility data.

Cc: [email protected]

> diff --git a/net/rose/rose_subr.c b/net/rose/rose_subr.c
> index 4dbc437a9e229..a902ddeddc5bd 100644
> --- a/net/rose/rose_subr.c
> +++ b/net/rose/rose_subr.c
> @@ -370,6 +370,9 @@ static int rose_parse_ccitt(unsigned char *p, struct rose_facilities_struct *fac
>  			if (l < 10 || l > 20)
>  				return -1;
>  
> +			if (len < 2 + l)
> +				return -1;
> +
>  			if (*p == FAC_CCITT_DEST_NSAP) {
>  				memcpy(&facilities->source_addr, p + 7, ROSE_ADDR_LEN);
>  				memcpy(callsign, p + 12,   l - 10);

Can asc2ax() read uninitialized stack memory here?

If a facility length of l = 17 is provided, memcpy() copies 7 bytes to the
callsign array, and the 8th byte is set to null, leaving the remaining
bytes uninitialized.

When asc2ax() processes this, it consumes the first 6 bytes in its loop.
The pointer then advances past the null byte, and the check if (*s != '\0')
will read the 9th byte, which contains uninitialized stack garbage.

Would zero-initializing the callsign array prevent this?

This regression wasn't introduced by this patch, but exists in the modified
function's loop termination logic:

net/rose/rose_subr.c:rose_parse_ccitt() {
    ...
    } while (*p != 0x00 && len > 0);
}

Due to short-circuit evaluation, if a facility consumes the exact remaining
bytes so that len becomes 0, will the loop dereference the pointer p before
checking if len > 0?

This would cause an out-of-bounds read past the valid facility data.

Should the condition be swapped to while (len > 0 && *p != 0x00)?

This regression wasn't introduced by this patch, but resides in the same
parsing sequence:

net/rose/rose_subr.c:rose_parse_facilities() {
    ...
    facilities_len = *p++;
    if (facilities_len == 0 || (unsigned int)facilities_len > packet_len)
    ...
}

Since packet_len represents the total remaining bytes including the 1-byte
length prefix itself, and the length byte has already been consumed, only
packet_len - 1 bytes actually remain for the facility data.

If an attacker crafts a packet where facilities_len == packet_len, will
this validation incorrectly pass?

The parsing loops will then expect packet_len bytes but only have
packet_len - 1 available, leading to an out-of-bounds read.

Could this check be updated to use facilities_len >= packet_len?

This regression wasn't introduced by this patch, but violates networking
subsystem safety rules regarding socket buffers.

Does the rose subsystem safely linearize socket buffers before dereferencing
packet headers?

Throughout the subsystem, including the parsing functions and routing logic,
packet offsets are accessed directly from skb->data based only on skb->len.

Since skb->len includes paged fragments, if a packet is fragmented such
that skb_headlen(skb) is smaller than the accessed offset, could
dereferencing skb->data trigger a page fault or read garbage memory?

Would adding pskb_may_pull() checks before accessing headers resolve this?