Re: [PATCH net] slip: reject VJ frames when no receive slots are allocated

[Simon Horman <horms@kernel.org>]

From: 'Simon Horman' <horms@kernel.org>

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
---
slip: reject VJ frames when no receive slots are allocated

This commit prevents a null pointer dereference by adding checks for
a NULL comp->rstate in slhc_uncompress() and slhc_remember(). It ensures
frames are rejected when no receive slots are allocated.

> diff --git a/drivers/net/slip/slhc.c b/drivers/net/slip/slhc.c
> index e3c785da3eef3..e67052bcab57b 100644
> --- a/drivers/net/slip/slhc.c
> +++ b/drivers/net/slip/slhc.c
> @@ -502,6 +502,10 @@ slhc_uncompress(struct slcompress *comp, unsigned char *icp, int isize)
>  
>  	/* We've got a compressed packet; read the change byte */
>  	comp->sls_i_compressed++;
> +	if (!comp->rstate) {
> +		comp->sls_i_error++;
> +		return 0;
> +	}

While reviewing this code, a few pre-existing issues were noticed in and
around slhc_uncompress().

Can slhc_uncompress() cause a heap buffer overflow?

It expands compressed packets by prepending reconstructed IP and TCP
headers, which can total up to 120 bytes if options are maximized. The
caller in slip.c assumes a maximum expansion of 80 bytes:

    if (count + 80 > sl->buffsize)

If a compressed packet is sized at sl->buffsize - 80, slhc_uncompress()
shifts the payload and copies the 120-byte headers, writing up to 37 bytes
past the dynamically allocated sl->rbuff chunk. Does this corrupt memory?

Also, does the decode() function read variable-length deltas without
bounds checking against the packet length isize?

A malformed packet with multiple change flags set could cause decode()
to read up to 15 bytes past the end of the packet data. Because the packet
buffer sl->rbuff is allocated via kmalloc without zero-initialization,
could this read uninitialized kernel heap memory and inject it into the
reconstructed TCP/IP headers, leaking it back to the network?

Finally, do slhc_compress() and slhc_uncompress() read and write the
16-bit TCP checksum using direct pointer casts?

drivers/net/slip/slhc.c:slhc_uncompress() {
    ...
    thp->check = *(__sum16 *)cp;
    ...
}

Since the compressed header varies dynamically in size, the cp pointer
frequently lands on odd byte boundaries. On architectures with strict
alignment requirements, could this direct dereference trigger an alignment
fault and kernel panic? Should the code use get_unaligned() and
put_unaligned() instead?

[ ... ]

> @@ -651,8 +655,9 @@ slhc_remember(struct slcompress *comp, unsigned char *icp, int isize)
>  
>  	/* The packet is shorter than a legal IP header.
>  	 * Also make sure isize is positive.
> +	 * Reject if no receive slots are configured (rstate is NULL).
>  	 */
> -	if (isize < (int)sizeof(struct iphdr)) {
> +	if (!comp->rstate || isize < (int)sizeof(struct iphdr)) {
>  runt:
>  		comp->sls_i_runt++;
>  		return slhc_toss(comp);

When dropping a packet due to a lack of configured receive slots
(rslots == 0), should this increment comp->sls_i_error instead of
comp->sls_i_runt?

Dropping a packet because rstate is missing is an internal configuration
state, not a runt packet.