netdev - Re: [PATCH net] can: af_can: fix NULL pointer dereference in can_rx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 28 Oct 2022 09:46:37 +0200
From:   Marc Kleine-Budde <mkl@...gutronix.de>
To:     Oliver Hartkopp <socketcan@...tkopp.net>
Cc:     Zhengchao Shao <shaozhengchao@...wei.com>,
        linux-can@...r.kernel.org, netdev@...r.kernel.org,
        davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com, linux@...pel-privat.de, weiyongjun1@...wei.com,
        yuehaibing@...wei.com
Subject: Re: [PATCH net] can: af_can: fix NULL pointer dereference in
 can_rx_register()

On 28.10.2022 09:13:09, Oliver Hartkopp wrote:
> Hello,
> 
> On 28.10.22 05:33, Zhengchao Shao wrote:
> > It causes NULL pointer dereference when testing as following:
> > (a) use syscall(__NR_socket, 0x10ul, 3ul, 0) to create netlink socket.
> > (b) use syscall(__NR_sendmsg, ...) to create bond link device and vxcan
> >      link device, and bind vxcan device to bond device (can also use
> >      ifenslave command to bind vxcan device to bond device).
> > (c) use syscall(__NR_socket, 0x1dul, 3ul, 1) to create CAN socket.
> > (d) use syscall(__NR_bind, ...) to bind the bond device to CAN socket.
> > 
> > The bond device invokes the can-raw protocol registration interface to
> > receive CAN packets. However, ml_priv is not allocated to the dev,
> > dev_rcv_lists is assigned to NULL in can_rx_register(). In this case,
> > it will occur the NULL pointer dereference issue.
> 
> I can see the problem and see that the patch makes sense for
> can_rx_register().
> 
> But for me the problem seems to be located in the bonding device.
> 
> A CAN interface with dev->type == ARPHRD_CAN *always* has the dev->ml_priv
> and dev->ml_priv_type set correctly.
> 
> I'm not sure if a bonding device does the right thing by just 'claiming' to
> be a CAN device (by setting dev->type to ARPHRD_CAN) but not taking care of
> being a CAN device and taking care of ml_priv specifics.
> 
> This might also be the case in other ml_priv use cases.
> 
> Would it probably make sense to blacklist CAN devices in bonding devices?

NACK - We had this discussion 2.5 years ago:

| https://lore.kernel.org/all/00000000000030dddb059c562a3f@google.com
| https://lore.kernel.org/all/20200130133046.2047-1-socketcan@hartkopp.net

...and davem pointed out:

| https://lore.kernel.org/all/20200226.202326.295871777946911500.davem@davemloft.net

On 26.02.2020 20:23:26, David Miller wrote:
[...]
> What I don't get is why the PF_CAN is blindly dereferencing a device
> assuming what is behind bond_dev->ml_priv.
> 
> If it assumes a device it access is CAN then it should check the
> device by comparing the netdev_ops or via some other means.
> 
> This restriction seems arbitrary.

With the addition of struct net_device::ml_priv_type in 4e096a18867a
("net: introduce CAN specific pointer in the struct net_device"), what
davem requested is now possible.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)