netdev - Re: [PATCH net] tls: prevent oversized sendfile() hangs by ignoring MSG

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: <YM0KR8IPGoSBgCl8@ubuntu-x1>
Date:   Fri, 18 Jun 2021 16:04:07 -0500
From:   Seth Forshee <seth.forshee@...onical.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     davem@...emloft.net, netdev@...r.kernel.org, borisp@...dia.com,
        john.fastabend@...il.com, daniel@...earbox.net, davejwatson@...com,
        ilyal@...lanox.com, aviadye@...lanox.com,
        Vadim Fedorenko <vfedorenko@...ek.ru>
Subject: Re: [PATCH net] tls: prevent oversized sendfile() hangs by ignoring
 MSG_MORE

On Fri, Jun 18, 2021 at 01:34:06PM -0700, Jakub Kicinski wrote:
> We got multiple reports that multi_chunk_sendfile test
> case from tls selftest fails. This was sort of expected,
> as the original fix was never applied (see it in the first
> Link:). The test in question uses sendfile() with count
> larger than the size of the underlying file. This will
> make splice set MSG_MORE on all sendpage calls, meaning
> TLS will never close and flush the last partial record.
> 
> Eric seem to have addressed a similar problem in
> commit 35f9c09fe9c7 ("tcp: tcp_sendpages() should call tcp_push() once")
> by introducing MSG_SENDPAGE_NOTLAST. Unlike MSG_MORE
> MSG_SENDPAGE_NOTLAST is not set on the last call
> of a "pipefull" of data (PIPE_DEF_BUFFERS == 16,
> so every 16 pages or whenever we run out of data).
> 
> Having a break every 16 pages should be fine, TLS
> can pack exactly 4 pages into a record, so for
> aligned reads there should be no difference,
> unaligned may see one extra record per sendpage().
> 
> Sticking to TCP semantics seems preferable to modifying
> splice, but we can revisit it if real life scenarios
> show a regression.
> 
> Reported-by: Vadim Fedorenko <vfedorenko@...ek.ru>
> Reported-by: Seth Forshee <seth.forshee@...onical.com>
> Link: https://lore.kernel.org/netdev/1591392508-14592-1-git-send-email-pooja.trivedi@stackpath.com/
> Fixes: 3c4d7559159b ("tls: kernel TLS support")
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>

With this patch the muilt_chunk_sendfile selftest passes. Thanks!

Tested-by: Seth Forshee <seth.forshee@...onical.com>