[PATCH v2] smsc95xx: Use skb_cow_head to deal with cloned skbs

[PATCH v2] smsc95xx: Use skb_cow_head to deal with cloned skbs

[James Hughes]

The driver was failing to check that the SKB wasn't cloned
before adding checksum data.
Replace existing handling to extend/copy the header buffer
with skb_cow_head.

Signed-off-by: James Hughes <james.hughes@raspberrypi.org>
---
Changes in v2
 - Changed skb_cow to skb_cow_head as suggested by netdev list


 drivers/net/usb/smsc95xx.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index df60c98..094f0ee 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -2067,13 +2067,9 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
 	/* We do not advertise SG, so skbs should be already linearized */
 	BUG_ON(skb_shinfo(skb)->nr_frags);
 
-	if (skb_headroom(skb) < overhead) {
-		struct sk_buff *skb2 = skb_copy_expand(skb,
-			overhead, 0, flags);
-		dev_kfree_skb_any(skb);
-		skb = skb2;
-		if (!skb)
-			return NULL;
+	/* Make writable and expand header space by overhead if required */
+	if (skb_cow_head(skb, overhead)) {
+		return NULL;
 	}
 
 	if (csum) {
-- 
2.9.3

Re: [PATCH v2] smsc95xx: Use skb_cow_head to deal with cloned skbs

[Eric Dumazet]

On Tue, 2017-04-18 at 18:17 +0100, James Hughes wrote:
> The driver was failing to check that the SKB wasn't cloned
> before adding checksum data.
> Replace existing handling to extend/copy the header buffer
> with skb_cow_head.
> 
> Signed-off-by: James Hughes <james.hughes@raspberrypi.org>
> ---
> Changes in v2
>  - Changed skb_cow to skb_cow_head as suggested by netdev list
> 
> 
>  drivers/net/usb/smsc95xx.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
> index df60c98..094f0ee 100644
> --- a/drivers/net/usb/smsc95xx.c
> +++ b/drivers/net/usb/smsc95xx.c
> @@ -2067,13 +2067,9 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
>  	/* We do not advertise SG, so skbs should be already linearized */
>  	BUG_ON(skb_shinfo(skb)->nr_frags);
>  
> -	if (skb_headroom(skb) < overhead) {
> -		struct sk_buff *skb2 = skb_copy_expand(skb,
> -			overhead, 0, flags);
> -		dev_kfree_skb_any(skb);
> -		skb = skb2;
> -		if (!skb)
> -			return NULL;
> +	/* Make writable and expand header space by overhead if required */
> +	if (skb_cow_head(skb, overhead)) {

I believe you still need to 
		dev_kfree_skb_any(skb);

> +		return NULL;
>  	}
>  
>  	if (csum) {

RE: [PATCH v2] smsc95xx: Use skb_cow_head to deal with cloned skbs

[Woojung.Huh]

> > @@ -2067,13 +2067,9 @@ static struct sk_buff *smsc95xx_tx_fixup(struct
> usbnet *dev,
> >  	/* We do not advertise SG, so skbs should be already linearized */
> >  	BUG_ON(skb_shinfo(skb)->nr_frags);
> >
> > -	if (skb_headroom(skb) < overhead) {
> > -		struct sk_buff *skb2 = skb_copy_expand(skb,
> > -			overhead, 0, flags);
> > -		dev_kfree_skb_any(skb);
> > -		skb = skb2;
> > -		if (!skb)
> > -			return NULL;
> > +	/* Make writable and expand header space by overhead if required
> */
> > +	if (skb_cow_head(skb, overhead)) {
> 
> I believe you still need to
> 		dev_kfree_skb_any(skb);
> 
I think caller of usbnet_start_xmit() takes care of free when return NULL.

- Woojung

Re: [PATCH v2] smsc95xx: Use skb_cow_head to deal with cloned skbs

[Eric Dumazet]

On Tue, 2017-04-18 at 22:09 +0000, Woojung.Huh@microchip.com wrote:
> > > @@ -2067,13 +2067,9 @@ static struct sk_buff *smsc95xx_tx_fixup(struct
> > usbnet *dev,
> > >  	/* We do not advertise SG, so skbs should be already linearized */
> > >  	BUG_ON(skb_shinfo(skb)->nr_frags);
> > >
> > > -	if (skb_headroom(skb) < overhead) {
> > > -		struct sk_buff *skb2 = skb_copy_expand(skb,
> > > -			overhead, 0, flags);
> > > -		dev_kfree_skb_any(skb);
> > > -		skb = skb2;
> > > -		if (!skb)
> > > -			return NULL;
> > > +	/* Make writable and expand header space by overhead if required
> > */
> > > +	if (skb_cow_head(skb, overhead)) {
> > 
> > I believe you still need to
> > 		dev_kfree_skb_any(skb);
> > 
> I think caller of usbnet_start_xmit() takes care of free when return NULL.

I do not think so. Here is the code in usbnet_start_xmit() :

if (info->tx_fixup) {
   skb = info->tx_fixup (dev, skb, GFP_ATOMIC);

   if (!skb) { // Note that skb is NULL now

       if (info->flags & FLAG_MULTI_PACKET)
             goto not_drop;
        netif_dbg(dev, tx_err, dev->net, "can't tx_fixup skb\n");
        goto drop;


If you really think about it (even before double checking the code in
usbnet_start_xmit()), that would have been a very serious bug.

Calling dev_kfree_skb_any(skb) in this fixup code, then later from
usbnet_start_xmit() would have been a double free, or use after free.

RE: [PATCH v2] smsc95xx: Use skb_cow_head to deal with cloned skbs

[Woojung.Huh]

> if (info->tx_fixup) {
>    skb = info->tx_fixup (dev, skb, GFP_ATOMIC);
> 
>    if (!skb) { // Note that skb is NULL now

You are right. skb  is return value of tx_fixup().

- Woojung

Re: [PATCH v2] smsc95xx: Use skb_cow_head to deal with cloned skbs

[James Hughes]

On 18 April 2017 at 23:46, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2017-04-18 at 22:09 +0000, Woojung.Huh@microchip.com wrote:
>> > > @@ -2067,13 +2067,9 @@ static struct sk_buff *smsc95xx_tx_fixup(struct
>> > usbnet *dev,
>> > >   /* We do not advertise SG, so skbs should be already linearized */
>> > >   BUG_ON(skb_shinfo(skb)->nr_frags);
>> > >
>> > > - if (skb_headroom(skb) < overhead) {
>> > > -         struct sk_buff *skb2 = skb_copy_expand(skb,
>> > > -                 overhead, 0, flags);
>> > > -         dev_kfree_skb_any(skb);
>> > > -         skb = skb2;
>> > > -         if (!skb)
>> > > -                 return NULL;
>> > > + /* Make writable and expand header space by overhead if required
>> > */
>> > > + if (skb_cow_head(skb, overhead)) {
>> >
>> > I believe you still need to
>> >             dev_kfree_skb_any(skb);
>> >
>> I think caller of usbnet_start_xmit() takes care of free when return NULL.
>
> I do not think so. Here is the code in usbnet_start_xmit() :
>
> if (info->tx_fixup) {
>    skb = info->tx_fixup (dev, skb, GFP_ATOMIC);
>
>    if (!skb) { // Note that skb is NULL now
>
>        if (info->flags & FLAG_MULTI_PACKET)
>              goto not_drop;
>         netif_dbg(dev, tx_err, dev->net, "can't tx_fixup skb\n");
>         goto drop;
>
>
> If you really think about it (even before double checking the code in
> usbnet_start_xmit()), that would have been a very serious bug.
>
> Calling dev_kfree_skb_any(skb) in this fixup code, then later from
> usbnet_start_xmit() would have been a double free, or use after free.
>
>

So, I still need to return NULL (as per the code this is replacing) to
indicate failure, but need to free the skb prior to return, as per
fragment below.

/* Make writable and expand header space by overhead if required */
if (skb_cow_head(skb, overhead)) {
   dev_kfree_skb_any(skb);
   return NULL;
}

Once confirmed, I'll do a another patch.

Re: [PATCH v2] smsc95xx: Use skb_cow_head to deal with cloned skbs

[James Hughes]

On 19 April 2017 at 09:33, James Hughes <james.hughes@raspberrypi.org> wrote:
> On 18 April 2017 at 23:46, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> On Tue, 2017-04-18 at 22:09 +0000, Woojung.Huh@microchip.com wrote:
>>> > > @@ -2067,13 +2067,9 @@ static struct sk_buff *smsc95xx_tx_fixup(struct
>>> > usbnet *dev,
>>> > >   /* We do not advertise SG, so skbs should be already linearized */
>>> > >   BUG_ON(skb_shinfo(skb)->nr_frags);
>>> > >
>>> > > - if (skb_headroom(skb) < overhead) {
>>> > > -         struct sk_buff *skb2 = skb_copy_expand(skb,
>>> > > -                 overhead, 0, flags);
>>> > > -         dev_kfree_skb_any(skb);
>>> > > -         skb = skb2;
>>> > > -         if (!skb)
>>> > > -                 return NULL;
>>> > > + /* Make writable and expand header space by overhead if required
>>> > */
>>> > > + if (skb_cow_head(skb, overhead)) {
>>> >
>>> > I believe you still need to
>>> >             dev_kfree_skb_any(skb);
>>> >
>>> I think caller of usbnet_start_xmit() takes care of free when return NULL.
>>
>> I do not think so. Here is the code in usbnet_start_xmit() :
>>
>> if (info->tx_fixup) {
>>    skb = info->tx_fixup (dev, skb, GFP_ATOMIC);
>>
>>    if (!skb) { // Note that skb is NULL now
>>
>>        if (info->flags & FLAG_MULTI_PACKET)
>>              goto not_drop;
>>         netif_dbg(dev, tx_err, dev->net, "can't tx_fixup skb\n");
>>         goto drop;
>>
>>
>> If you really think about it (even before double checking the code in
>> usbnet_start_xmit()), that would have been a very serious bug.
>>
>> Calling dev_kfree_skb_any(skb) in this fixup code, then later from
>> usbnet_start_xmit() would have been a double free, or use after free.
>>
>>
>
> So, I still need to return NULL (as per the code this is replacing) to
> indicate failure, but need to free the skb prior to return, as per
> fragment below.
>
> /* Make writable and expand header space by overhead if required */
> if (skb_cow_head(skb, overhead)) {
>    dev_kfree_skb_any(skb);
>    return NULL;
> }
>
> Once confirmed, I'll do a another patch.

Actually, having checked the code path, doing the free here does look
like the correct operation. Returning NULL indicates to the caller
that the call has failed, but also means the skb is not deallocated
later on. It's seems rather a clunky mechanism for the driver to have
to deallocate, but I guess that's 'just the way it is'.

I'll do another patch.