netdev - Re: [PATCH net-next V1 1/4] net/mlx5e: Restore the skb data pointer after xmit is finished

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20151217.152104.2240043588504838923.davem@davemloft.net>
Date:	Thu, 17 Dec 2015 15:21:04 -0500 (EST)
From:	David Miller <davem@...emloft.net>
To:	saeedm@...lanox.com
Cc:	netdev@...r.kernel.org, ogerlitz@...lanox.com, eranbe@...lanox.com,
	talal@...lanox.com, richardcochran@...il.com
Subject: Re: [PATCH net-next V1 1/4] net/mlx5e: Restore the skb data
 pointer after xmit is finished

From: Saeed Mahameed <saeedm@...lanox.com>
Date: Thu, 17 Dec 2015 14:35:32 +0200

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
> index 1341b1d..0fcfe64 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
> @@ -165,6 +165,7 @@ static netdev_tx_t mlx5e_sq_xmit(struct mlx5e_sq *sq, struct sk_buff *skb)
>  	struct mlx5_wqe_eth_seg  *eseg = &wqe->eth;
>  	struct mlx5_wqe_data_seg *dseg;
>  
> +	unsigned char *skb_data_orig = skb->data;
>  	u8  opcode = MLX5_OPCODE_SEND;
>  	dma_addr_t dma_addr = 0;
>  	bool bf = false;
> @@ -263,6 +264,7 @@ static netdev_tx_t mlx5e_sq_xmit(struct mlx5e_sq *sq, struct sk_buff *skb)
>  	cseg->opmod_idx_opcode = cpu_to_be32((sq->pc << 8) | opcode);
>  	cseg->qpn_ds           = cpu_to_be32((sq->sqn << 8) | ds_cnt);
>  
> +	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.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html