Re: [PATCH v2 net] tun: remove skb access after netif_receive_skb

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Dec 03, 2018 at 06:09:24PM +0900, Prashant Bhole wrote:
> In tun.c skb->len was accessed while doing stats accounting after a
> call to netif_receive_skb. We can not access skb after this call
> because buffers may be dropped.
> 
> The fix for this bug would be to store skb->len in local variable and
> then use it after netif_receive_skb(). IMO using xdp data size for
> accounting bytes will be better because input for tun_xdp_one() is
> xdp_buff.
> 
> Hence this patch:
> - fixes a bug by removing skb access after netif_receive_skb()
> - uses xdp data size for accounting bytes
> 
> [613.019057] BUG: KASAN: use-after-free in tun_sendmsg+0x77c/0xc50 [tun]
> [613.021062] Read of size 4 at addr ffff8881da9ab7c0 by task vhost-1115/1155
> [613.023073]
> [613.024003] CPU: 0 PID: 1155 Comm: vhost-1115 Not tainted 4.20.0-rc3-vm+ #232
> [613.026029] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
> [613.029116] Call Trace:
> [613.031145]  dump_stack+0x5b/0x90
> [613.032219]  print_address_description+0x6c/0x23c
> [613.034156]  ? tun_sendmsg+0x77c/0xc50 [tun]
> [613.036141]  kasan_report.cold.5+0x241/0x308
> [613.038125]  tun_sendmsg+0x77c/0xc50 [tun]
> [613.040109]  ? tun_get_user+0x1960/0x1960 [tun]
> [613.042094]  ? __isolate_free_page+0x270/0x270
> [613.045173]  vhost_tx_batch.isra.14+0xeb/0x1f0 [vhost_net]
> [613.047127]  ? peek_head_len.part.13+0x90/0x90 [vhost_net]
> [613.049096]  ? get_tx_bufs+0x5a/0x2c0 [vhost_net]
> [613.051106]  ? vhost_enable_notify+0x2d8/0x420 [vhost]
> [613.053139]  handle_tx_copy+0x2d0/0x8f0 [vhost_net]
> [613.053139]  ? vhost_net_buf_peek+0x340/0x340 [vhost_net]
> [613.053139]  ? __mutex_lock+0x8d9/0xb30
> [613.053139]  ? finish_task_switch+0x8f/0x3f0
> [613.053139]  ? handle_tx+0x32/0x120 [vhost_net]
> [613.053139]  ? mutex_trylock+0x110/0x110
> [613.053139]  ? finish_task_switch+0xcf/0x3f0
> [613.053139]  ? finish_task_switch+0x240/0x3f0
> [613.053139]  ? __switch_to_asm+0x34/0x70
> [613.053139]  ? __switch_to_asm+0x40/0x70
> [613.053139]  ? __schedule+0x506/0xf10
> [613.053139]  handle_tx+0xc7/0x120 [vhost_net]
> [613.053139]  vhost_worker+0x166/0x200 [vhost]
> [613.053139]  ? vhost_dev_init+0x580/0x580 [vhost]
> [613.053139]  ? __kthread_parkme+0x77/0x90
> [613.053139]  ? vhost_dev_init+0x580/0x580 [vhost]
> [613.053139]  kthread+0x1b1/0x1d0
> [613.053139]  ? kthread_park+0xb0/0xb0
> [613.053139]  ret_from_fork+0x35/0x40
> [613.088705]
> [613.088705] Allocated by task 1155:
> [613.088705]  kasan_kmalloc+0xbf/0xe0
> [613.088705]  kmem_cache_alloc+0xdc/0x220
> [613.088705]  __build_skb+0x2a/0x160
> [613.088705]  build_skb+0x14/0xc0
> [613.088705]  tun_sendmsg+0x4f0/0xc50 [tun]
> [613.088705]  vhost_tx_batch.isra.14+0xeb/0x1f0 [vhost_net]
> [613.088705]  handle_tx_copy+0x2d0/0x8f0 [vhost_net]
> [613.088705]  handle_tx+0xc7/0x120 [vhost_net]
> [613.088705]  vhost_worker+0x166/0x200 [vhost]
> [613.088705]  kthread+0x1b1/0x1d0
> [613.088705]  ret_from_fork+0x35/0x40
> [613.088705]
> [613.088705] Freed by task 1155:
> [613.088705]  __kasan_slab_free+0x12e/0x180
> [613.088705]  kmem_cache_free+0xa0/0x230
> [613.088705]  ip6_mc_input+0x40f/0x5a0
> [613.088705]  ipv6_rcv+0xc9/0x1e0
> [613.088705]  __netif_receive_skb_one_core+0xc1/0x100
> [613.088705]  netif_receive_skb_internal+0xc4/0x270
> [613.088705]  br_pass_frame_up+0x2b9/0x2e0
> [613.088705]  br_handle_frame_finish+0x2fb/0x7a0
> [613.088705]  br_handle_frame+0x30f/0x6c0
> [613.088705]  __netif_receive_skb_core+0x61a/0x15b0
> [613.088705]  __netif_receive_skb_one_core+0x8e/0x100
> [613.088705]  netif_receive_skb_internal+0xc4/0x270
> [613.088705]  tun_sendmsg+0x738/0xc50 [tun]
> [613.088705]  vhost_tx_batch.isra.14+0xeb/0x1f0 [vhost_net]
> [613.088705]  handle_tx_copy+0x2d0/0x8f0 [vhost_net]
> [613.088705]  handle_tx+0xc7/0x120 [vhost_net]
> [613.088705]  vhost_worker+0x166/0x200 [vhost]
> [613.088705]  kthread+0x1b1/0x1d0
> [613.088705]  ret_from_fork+0x35/0x40
> [613.088705]
> [613.088705] The buggy address belongs to the object at ffff8881da9ab740
> [613.088705]  which belongs to the cache skbuff_head_cache of size 232
> 
> Fixes: 043d222f93ab ("tuntap: accept an array of XDP buffs through sendmsg()")
> Reviewed-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
> Acked-by: Jason Wang <jasowang@redhat.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
> v1 -> v2:
> No change. Reposted due to patchwork status.
> 
>  drivers/net/tun.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index e244f5d7512a..6e388792c0a8 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -2385,6 +2385,7 @@ static int tun_xdp_one(struct tun_struct *tun,
>  		       struct tun_file *tfile,
>  		       struct xdp_buff *xdp, int *flush)
>  {
> +	unsigned int datasize = xdp->data_end - xdp->data;
>  	struct tun_xdp_hdr *hdr = xdp->data_hard_start;
>  	struct virtio_net_hdr *gso = &hdr->gso;
>  	struct tun_pcpu_stats *stats;
> @@ -2461,7 +2462,7 @@ static int tun_xdp_one(struct tun_struct *tun,
>  	stats = get_cpu_ptr(tun->pcpu_stats);
>  	u64_stats_update_begin(&stats->syncp);
>  	stats->rx_packets++;
> -	stats->rx_bytes += skb->len;
> +	stats->rx_bytes += datasize;
>  	u64_stats_update_end(&stats->syncp);
>  	put_cpu_ptr(stats);
>  
> -- 
> 2.17.2
>