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

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

[Weiming Shi]

slhc_init() allows rslots == 0 and in that case skips the allocation
of comp->rstate, leaving it NULL. Because the struct is zero-initialized
by kzalloc, comp->rslot_limit is also 0.

The receive-side entry points slhc_uncompress() and slhc_remember()
only compare a packet's slot index against rslot_limit, so slot 0
passes the bounds check even though no receive state array exists.
Any VJ-compressed or VJ-uncompressed packet that selects slot 0 then
dereferences the NULL rstate pointer.

This can be reached through PPP by issuing PPPIOCSMAXCID with a value
whose upper 16 bits, after arithmetic right shift, yield -1, making
val2 + 1 == 0 and thus rslots == 0.

 Oops: general protection fault, probably for non-canonical address 0xdffffc0000000000: 0000 [#1] SMP KASAN NOPTI
 KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
 RIP: 0010:slhc_uncompress (drivers/net/slip/slhc.c:519)
 Call Trace:
  <TASK>
  ppp_receive_nonmp_frame (drivers/net/ppp/ppp_generic.c:2466)
  ppp_input (drivers/net/ppp/ppp_generic.c:2359)
  ppp_async_process (drivers/net/ppp/ppp_async.c:492)
  tasklet_action_common (kernel/softirq.c:926)
  handle_softirqs (kernel/softirq.c:623)
  run_ksoftirqd (kernel/softirq.c:1055)
  smpboot_thread_fn (kernel/smpboot.c:160)
  kthread (kernel/kthread.c:436)
  ret_from_fork (arch/x86/kernel/process.c:164)
  </TASK>

Add a NULL check on comp->rstate at the entry of slhc_uncompress() and
slhc_remember() so that frames are rejected when no receive slots exist.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: Xiang Mei <xmei5@asu.edu>
Signed-off-by: Weiming Shi <bestswngs@gmail.com>
---
 drivers/net/slip/slhc.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/slip/slhc.c b/drivers/net/slip/slhc.c
index e3c785da3eef..e67052bcab57 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;
+	}
 	if(isize < 3){
 		comp->sls_i_error++;
 		return 0;
@@ -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);
-- 
2.43.0

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

[Simon Horman]

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.

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

[Weiming Shi]

On 26-04-14 14:41, Simon Horman wrote:
> 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.

Thanks, Simon. I will send v2 patch.

I also reproduced the three pre-existing issues you pointed out
(one via KASAN, the other two under GDB) and will send patches for
them separately.

Best regards,
Weiming Shi