Re: [PATCH net v2] slip: reject VJ receive packets on instances with no rstate array

[Weiming Shi <bestswngs@gmail.com>]

On 26-04-18 13:39, Simon Horman wrote:
> On Thu, Apr 16, 2026 at 04:41:31AM +0800, Weiming Shi wrote:
> > slhc_init() accepts rslots == 0 as a valid configuration, with the
> > documented meaning of 'no receive compression'. In that case the
> > allocation loop in slhc_init() is skipped, so comp->rstate stays
> > NULL and comp->rslot_limit stays 0 (from the kzalloc of struct
> > slcompress).
> > 
> > The receive helpers do not defend against that configuration.
> > slhc_uncompress() dereferences comp->rstate[x] when the VJ header
> > carries an explicit connection ID, and slhc_remember() later assigns
> > cs = &comp->rstate[...] after only comparing the packet's slot number
> > to comp->rslot_limit. Because rslot_limit is 0, slot 0 passes the
> > range check, and the code dereferences a NULL rstate.
> > 
> > The configuration is reachable in-tree through PPP. PPPIOCSMAXCID
> > stores its argument in a signed int, and (val >> 16) uses arithmetic
> > shift. Passing 0xffff0000 therefore sign-extends to -1, so val2 + 1
> > is 0 and ppp_generic.c ends up calling slhc_init(0, 1). Because
> > /dev/ppp open is gated by ns_capable(CAP_NET_ADMIN), the whole path
> > is reachable from an unprivileged user namespace. Once the malformed
> > VJ state is installed, any inbound VJ-compressed or VJ-uncompressed
> > frame that selects slot 0 crashes the kernel in softirq context:
> > 
> >  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>
> > 
> > Reject the receive side on such instances instead of touching rstate.
> > slhc_uncompress() falls through to its existing 'bad' label, which
> > bumps sls_i_error and enters the toss state. slhc_remember() mirrors
> > that with an explicit sls_i_error increment followed by slhc_toss();
> > the sls_i_runt counter is not used here because a missing rstate is
> > an internal configuration state, not a runt packet.
> > 
> > The transmit path is unaffected: the only in-tree caller that picks
> > rslots from userspace (ppp_generic.c) still supplies tslots >= 1, and
> > slip.c always calls slhc_init(16, 16), so comp->tstate remains valid
> > and slhc_compress() continues to work.
> > 
> > Fixes: b5451d783ade ("slip: Move the SLIP drivers")
> 
> AI review points out that the cited commit moves code but doesn't
> add this bug.
> 
> It seems to me that this bug has existed since the beginning of git
> history. If so, the Fixes tag should be:
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> 
> > Reported-by: Xiang Mei <xmei5@asu.edu>
> > Signed-off-by: Weiming Shi <bestswngs@gmail.com>
> > ---
> > v2:
> > - slhc_remember(): use sls_i_error instead of sls_i_runt for the
> >   missing-rstate case; it is a configuration error, not a runt packet
> >   (Simon).
> > - slhc_uncompress(): goto bad instead of returning 0, so the instance
> >   also enters SLF_TOSS on the first rejected frame.
> 
> Otherwise this looks good to me:
> 
> Reviewed-by: Simon Horman <horms@kernel.org>
> 
> 
> I do note that Sashiko flags some other problems in this code.
> I do not think that needs to delay progress of this patch.
> But you may wish to look into them as follow-up work.

Thanks for your review. 

I've already sent two follow-up patches for the decode()/pull16() 
bounds-checking issues:

    [PATCH net] slip: fix slab-out-of-bounds write in slhc_uncompress()
    https://lore.kernel.org/netdev/20260415213359.335657-2-bestswngs@gmail.com/

    [PATCH net] slip: bound decode() reads against the compressed packet length
    https://lore.kernel.org/netdev/20260416100147.531855-5-bestswngs@gmail.com/

Best regards,
Weiming Shi