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@pengutronix.de>
Sent: Wednesday, August 28, 2019 1:03 AM
To: Srinivas Neeli <redacted>; wg@grandegger.com
Cc: Srinivas Goud <redacted>; Naga Sureshkumar Relli
[off-list ref]; Appana Durga Kedareswara Rao
[off-list ref]; linux-can@vger.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:

quoted

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.

quoted

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   |

On 8/28/19 1:02 PM, Srinivas Neeli wrote:

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);

Now I get what you mean.

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

I think the can_put_echo_skb() API needs clarification. The driver is
not allowed to touch the skb any more after can_put_echo_skb(). We
should review the driver for this.

thanks,
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   |