netdev - Re: [net v4 05/10] cxgb4/ch_ktls: creating skbs causes panic

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]

Message-ID: <20201104115833.2ff3100a@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Wed, 4 Nov 2020 11:58:33 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     rohit maheshwari <rohitm@...lsio.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, secdev@...lsio.com
Subject: Re: [net v4 05/10] cxgb4/ch_ktls: creating skbs causes panic

On Wed, 4 Nov 2020 22:23:14 +0530 rohit maheshwari wrote:
> On 04/11/20 2:16 AM, Jakub Kicinski wrote:
> > On Fri, 30 Oct 2020 23:32:20 +0530 Rohit Maheshwari wrote:  
> >> Creating SKB per tls record and freeing the original one causes
> >> panic. There will be race if connection reset is requested. By
> >> freeing original skb, refcnt will be decremented and that means,
> >> there is no pending record to send, and so tls_dev_del will be
> >> requested in control path while SKB of related connection is in
> >> queue.
> >>   Better approach is to use same SKB to send one record (partial
> >> data) at a time. We still have to create a new SKB when partial
> >> last part of a record is requested.
> >>   This fix introduces new API cxgb4_write_partial_sgl() to send
> >> partial part of skb. Present cxgb4_write_sgl can only provide
> >> feasibility to start from an offset which limits to header only
> >> and it can write sgls for the whole skb len. But this new API
> >> will help in both. It can start from any offset and can end
> >> writing in middle of the skb.  
> > You never replied to my question on v2.
> >
> > If the problem is that the socket gets freed, why don't you make the
> > new skb take a reference on the socket?
> >
> > 650 LoC is really a rather large fix.  
> This whole skb alloc and copy record frags was written under the
> assumption that there will be zero data copy (no linear skb was
> expected) but that isn't correct. Continuing with the same change
> requires more checks and will be more complicated. That's why I
> made this change. I think using same SKB to send out multiple
> records is better than allocating new SKB every time.

Bug fixes are not where code should be restructured.

Please produce a minimal fix.