Re: Possible unsafe usage of skb->cb in virtio-net

From: Jason Wang <jasowang@redhat.com>
To: "Michael S. Tsirkin" <mst@redhat.com>,
	Ilya Lesokhin <ilyal@mellanox.com>
Cc: netdev@vger.kernel.org, Willem de Bruijn <willemb@google.com>,
	David Miller <davem@davemloft.net>,
	virtualization@lists.linux-foundation.org
Subject: Re: Possible unsafe usage of skb->cb in virtio-net
Date: Tue, 19 Dec 2017 17:06:25 +0800	[thread overview]
Message-ID: <2cb1512d-2db5-34a5-21c0-dd3fa2db8ad1@redhat.com> (raw)
In-Reply-To: <20171102142758-mutt-send-email-mst@kernel.org>

On 2017年11月02日 21:01, Michael S. Tsirkin wrote:
> On Thu, Nov 02, 2017 at 11:40:36AM +0000, Ilya Lesokhin wrote:
>> Hi,
>> I've noticed that the virtio-net uses skb->cb.
>>
>> I don't know all the detail by my understanding is it caused problem with the mlx5 driver
>> and was fixed here:
>> https://github.com/torvalds/linux/commit/34802a42b3528b0e18ea4517c8b23e1214a09332
>>
>> Thanks,
>> Ilya
> Thanks a lot for the pointer.
>
> I think this was in response to this:
> https://patchwork.ozlabs.org/patch/558324/
>
>>> +     skb_push(skb, skb->data - skb_data_orig);
>>>        sq->skb[pi] = skb;
>>>
>>>        MLX5E_TX_SKB_CB(skb)->num_wqebbs = DIV_ROUND_UP(ds_cnt,
>> And in the middle of this we have:
>>
>>                  skb_pull_inline(skb, ihs);
>>
>> This is looks illegal.
>>
>> You must not modify the data pointers of any SKB that you receive for
>> sending via ->ndo_start_xmit() unless you know that absolutely you are
>> the one and only reference that exists to that SKB.
>>
>> And exactly for the case you are trying to "fix" here, you do not.  If
>> the SKB is cloned, or has an elevated users count, someone else can be
>> looking at it exactly at the same time you are messing with the data
>> pointers.
>>
>> I bet mlx4 has this bug too.
>>
>> You must fix this properly, by keeping track of an offset or similar
>> internally to your driver, rather than changing the SKB data pointers.
> What virtio does is this:
>
>          can_push = vi->any_header_sg &&
>                  !((unsigned long)skb->data & (__alignof__(*hdr) - 1)) &&
>                  !skb_header_cloned(skb) && skb_headroom(skb) >= hdr_len;
>          /* Even if we can, don't push here yet as this would skew
>           * csum_start offset below. */
>          if (can_push)
>                  hdr = (struct virtio_net_hdr_mrg_rxbuf *)(skb->data - hdr_len);
>          else
>                  hdr = skb_vnet_hdr(skb);
>
>
> This doesn't change the data pointers in a cloned skb but it does change the cb.
> Is it true that it's illegal to touch the cb in a cloned skb then?
>

I think not.

skb_clone() call __skb_copy_header() which did:

     memcpy(new->cb, old->cb, sizeof(old->cb));

Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization