Re: [tipc-discussion] [PATCH net] tipc: do not write skb_shinfo frags when doing decrytion

[Jon M. <jm...@re...>]

On 23/07/2021 13:43, Xin Long wrote:
> One skb's skb_shinfo frags are not writable, and they can be shared with
> other skbs' like by pskb_copy(). To write the frags may cause other skb's
> data crash.
> 
> So before doing en/decryption, skb_cow_data() should always be called for
> a cloned or nonlinear skb if req dst is using the same sg as req src.
> While at it, the likely branch can be removed, as it will be covered
> by skb_cow_data().
> 
> Note that esp_input() has the same issue, and I will fix it in another
> patch. tipc_aead_encrypt() doesn't have this issue, as it only processes
> linear data in the unlikely branch.
> 
> Fixes: fc1b6d6de220 ("tipc: introduce TIPC encryption & authentication")
> Reported-by: Shuang Li <sh...@re...>
> Signed-off-by: Xin Long <luc...@gm...>
> ---
>   net/tipc/crypto.c | 14 ++++----------
>   1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
> index e5c43d4d5a75..c9391d38de85 100644
> --- a/net/tipc/crypto.c
> +++ b/net/tipc/crypto.c
> @@ -898,16 +898,10 @@ static int tipc_aead_decrypt(struct net *net, struct tipc_aead *aead,
>   	if (unlikely(!aead))
>   		return -ENOKEY;
>   
> -	/* Cow skb data if needed */
> -	if (likely(!skb_cloned(skb) &&
> -		   (!skb_is_nonlinear(skb) || !skb_has_frag_list(skb)))) {
> -		nsg = 1 + skb_shinfo(skb)->nr_frags;
> -	} else {
> -		nsg = skb_cow_data(skb, 0, &unused);
> -		if (unlikely(nsg < 0)) {
> -			pr_err("RX: skb_cow_data() returned %d\n", nsg);
> -			return nsg;
> -		}
> +	nsg = skb_cow_data(skb, 0, &unused);
> +	if (unlikely(nsg < 0)) {
> +		pr_err("RX: skb_cow_data() returned %d\n", nsg);
> +		return nsg;
>   	}
>   
>   	/* Allocate memory for the AEAD operation */
> 
Acked-by: Jon Maloy <jm...@re...>