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

[Weiming Shi <bestswngs@gmail.com>]

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