netdev - Re: [PATCH net-next v3 7/7] tls: rx: do not use the standard strparser

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Thu, 4 Aug 2022 09:13:37 +0300
From:   Tariq Toukan <ttoukan.linux@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org, edumazet@...gle.com, pabeni@...hat.com,
        borisp@...dia.com, john.fastabend@...il.com, maximmi@...dia.com,
        tariqt@...dia.com, vfedorenko@...ek.ru,
        Ran Rozenstein <ranro@...dia.com>,
        "gal@...dia.com" <gal@...dia.com>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH net-next v3 7/7] tls: rx: do not use the standard
 strparser



On 8/4/2022 4:24 AM, Jakub Kicinski wrote:
> On Tue, 2 Aug 2022 17:54:01 +0300 Tariq Toukan wrote:
>>    [  407.589886] RIP: 0010:tls_device_decrypted+0x7a/0x2e0
> 
> Sorry, got distracted yesterday. This?

I also had issues yesterday with the decode script.
For some reason, it didn't work for me.
Probably a missing env variable, or a kernel config.

...
? tls_rx_rec_wait (tls_sw.c:?)
tls_rx_one_record (tls_sw.c:?)
tls_sw_recvmsg (??:?)
? __wait_for_common (build_utility.c:?)
? usleep_range_state (??:?)
inet6_recvmsg (??:?)
____sys_recvmsg (socket.c:?)
? _copy_from_user (??:?)
? iovec_from_user (??:?)
___sys_recvmsg (socket.c:?)
WARNING! Modules path isn't set, but is needed to parse this symbol
? post_static_params+0x15c/0x320 mlx5_core
WARNING! Modules path isn't set, but is needed to parse this symbol
? mlx5e_ktls_add_rx+0x3d1/0x5d0 mlx5_core
WARNING! Modules path isn't set, but is needed to parse this symbol
? mlx5e_ktls_add_rx+0x45d/0x5d0 mlx5_core
...

> 
> --->8--------------------
> tls: rx: device: bound the frag walk
> 
> We can't do skb_walk_frags() on the input skbs, because
> the input skbs is really just a pointer to the tcp read
> queue. We need to bound the "is decrypted" check by the
> amount of data in the message.
> 
> Note that the walk in tls_device_reencrypt() is after a
> CoW so the skb there is safe to walk. Actually in the
> current implementation it can't have frags at all, but
> whatever, maybe one day it will.
> 
> Reported-by: Tariq Toukan <tariqt@...dia.com>
> Fixes: 84c61fe1a75b ("tls: rx: do not use the standard strparser")
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
>   net/tls/tls_device.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
> index e3e6cf75aa03..6ed41474bdf8 100644
> --- a/net/tls/tls_device.c
> +++ b/net/tls/tls_device.c
> @@ -984,11 +984,17 @@ int tls_device_decrypted(struct sock *sk, struct tls_context *tls_ctx)
>   	int is_decrypted = skb->decrypted;
>   	int is_encrypted = !is_decrypted;
>   	struct sk_buff *skb_iter;
> +	int left;
>   
> +	left = rxm->full_len - skb->len;
>   	/* Check if all the data is decrypted already */
> -	skb_walk_frags(skb, skb_iter) {
> +	skb_iter = skb_shinfo(skb)->frag_list;
> +	while (skb_iter && left > 0) {
>   		is_decrypted &= skb_iter->decrypted;
>   		is_encrypted &= !skb_iter->decrypted;
> +
> +		left -= skb_iter->len;
> +		skb_iter = skb_iter->next;
>   	}
>   
>   	trace_tls_device_decrypted(sk, tcp_sk(sk)->copied_seq - rxm->full_len,

Thanks!
Testing it. I'll update.