netdev - RE: Query on possible bug in the can_create_echo

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-next>] [day] [month] [year] [list]

Date:   Wed, 28 Aug 2019 11:02:43 +0000
From:   Srinivas Neeli <sneeli@...inx.com>
To:     Marc Kleine-Budde <mkl@...gutronix.de>,
        "wg@...ndegger.com" <wg@...ndegger.com>
CC:     Srinivas Goud <sgoud@...inx.com>,
        Naga Sureshkumar Relli <nagasure@...inx.com>,
        Appana Durga Kedareswara Rao <appanad@...inx.com>,
        "linux-can@...r.kernel.org" <linux-can@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: Query on possible bug in the can_create_echo_skb() API

Hi,

Case 1:
can_put_echo_skb(); -> skb = can_create_echo_skb(skb); -> return skb;

In can_create_echo_skb() not using the shared_skb, so we are returning the old skb.
Storing the return value in "skb". But it's a pointer, for storing that need double pointer.
Instead of double-pointer using a single pointer. In this scenario it's ok , we are returning the same SKB.

Case 2:
can_put_echo_skb(skb, ndev, priv->tx_head % priv->tx_max); -> skb = can_create_echo_skb(skb); -> can_skb_set_owner(nskb, skb->sk); - Returning nskb;

shared_skb scenario:
In share-skb case “can_create_echo_skb(skb);”  returning "new skb". For storing new skb need a double pointer.

Providing an example for overcoming above issue.
Example:
can_put_echo_skb(struct sk_buff **skb,struct net_device *dev,unsigned int idx);

If you ok with this change, I will send a patch.


Thanks
Srinivas Neeli

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@...gutronix.de>
> Sent: Wednesday, August 28, 2019 1:03 AM
> To: Srinivas Neeli <sneeli@...inx.com>; wg@...ndegger.com
> Cc: Srinivas Goud <sgoud@...inx.com>; Naga Sureshkumar Relli
> <nagasure@...inx.com>; Appana Durga Kedareswara Rao
> <appanad@...inx.com>; linux-can@...r.kernel.org
> Subject: Re: Query on possible bug in the can_create_echo_skb() API
> 
> Hello Srinivas Neeli,
> 
> please don't send HTML messages to the kernel mailinglists.
> 
> On 8/21/19 12:51 PM, Srinivas Neeli wrote:
> > While walking through the CAN core layer dev.c file in the
> > can_put_echo_skb() API [1], Seems to be there is a race condition in
> > the
> > can_create_echo_skb() API, more details below
> >
> > If the skb is a shared skb, we are overwriting the skb pointer [2] in
> > the can_create_echo_skb() API and returning the new skb back.
> 
> Where and how is the skb pointer overwritten? Can you explain a bit more.
> 
> > If the core layer/drivers use this skb it is not valid any more (it
> > may lead to crash/oops).
> >
> >
> >
> > A possible solution for this issue would make the function input
> > argument should be double-pointer.
> >
> > Please correct me if my analyzation is wrong.
> 
> Can you provide a patch of your proposed changes?
> 
> regards,
> Marc
> 
> --
> Pengutronix e.K.                  | Marc Kleine-Budde           |
> Industrial Linux Solutions        | Phone: +49-231-2826-924     |
> Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |