Re: [PATCH net-next V2] can: j1939: fix uaf warning in j1939_session_destroy

[Oleksij Rempel]

Hi Sabyrzhan,

On Fri, Oct 11, 2024 at 06:41:24PM +0500, Sabyrzhan Tasbolatov wrote:
> On Thu, 8 Aug 2024 19:07:55 +0800, Edward Adam Davis wrote:
> > On Thu, 8 Aug 2024 09:49:18 +0200, Oleksij Rempel wrote:
> > > > the skb to the queue and increase the skb reference count through it.
> > > > 
> > > > Reported-and-tested-by: syzbot+ad601904231505ad6617@xxxxxxxxxxxxxxxxxxxxxxxxx
> > > > Closes: https://syzkaller.appspot.com/bug?extid=ad601904231505ad6617
> > > > Signed-off-by: Edward Adam Davis <eadavis@xxxxxx>
> > > 
> > > This patch breaks j1939.
> > > The issue can be reproduced by running following commands:
> > I tried to reproduce the problem using the following command, but was 
> > unsuccessful. Prompt me to install j1939cat and j1939acd, and there are
> > some other errors.
> > 
> > Can you share the logs from when you reproduced the problem?
 
ah, i was on vacation and it went under my radar, sorry :(

> Hello,
> 
> Here is the log of can-tests/j1939/run_all.sh:
> 
> # ip link add type vcan
> # ip l s dev vcan0 up
> # ./run_all.sh vcan0 vcan0
> ##############################################
> run: j1939_ac_100k_dual_can.sh
> generate random data for the test
> 1+0 records in
> 1+0 records out
> 102400 bytes (102 kB, 100 KiB) copied, 0.00191192 s, 53.6 MB/s
> start j1939acd and j1939cat on vcan0
> 8321
> 8323
> start j1939acd and j1939cat on vcan0
> [  132.211317][ T8326] vcan0: tx drop: invalid sa for name 0x0000000011223340
> j1939cat: j1939cat_send_one: transfer error: -99: Cannot assign requested address
> 
> It fails here:
> https://github.com/linux-can/can-tests/blob/master/j1939/j1939_ac_100k_dual_can.sh#L70

I assume it is just secondary fail, it probably failed on address claim
stage in j1939acd, so the j1939cat was not able to start transfer due to
missing (not claimed) address.

> The error message is printed in this condition:
> https://elixir.bootlin.com/linux/v6.12-rc2/source/net/can/j1939/address-claim.c#L104-L108
> 
> I've applied your patch on the current 6.12.0-rc2 and the syzkaller C repro
> doesn't trigger WARNING uaf, refcount anymore though.

Yes, because transfer protocol is broken now. 

> == Offtopic:
> I wonder if can-tests/j1939 should be refactored from shell to C tests in the
> same linux-can/can-tests repository (or even migrate to KUnit tests)
> to improve debugging, test coverage. I'd like to understand which syscalls
> and params are used j1939cat and j1939acd utils -- currently, tracing with
> strace and trace-cmd (ftrace).

I have nothing against it, some of them I implemented in C:
https://github.com/linux-can/can-tests/blob/master/j1939/tst-j1939-ac.c#L1160

Right now I do not have enough time to port it, but I can support anyone
who is willing to do it.

Best Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |