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.toshi...@lab.ntt.co.jp>
> Signed-off-by: Prashant Bhole <bhole_prashant...@lab.ntt.co.jp>
> Acked-by: Jason Wang <jasow...@redhat.com>
Acked-by: Michael S. Tsirkin <m...@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
>
