2023-08-02 17:31:18

by Florent Revest

[permalink] [raw]
Subject: [RFC 0/1] crypto: Avoid a sleepable operation when freeing a SCTP socket

Hi!

I found that the following program reliably reproduces a "BUG: sleeping function
called from invalid context" backtrace in crypto code:

#include <sys/socket.h>
#include <linux/in.h>
#include <linux/if_alg.h>

int main(void)
{
int fd1 = socket(AF_INET6, SOCK_STREAM, IPPROTO_SCTP);
if (fd1 == -1)
return 1;
listen(fd1, 1);

int fd2 = socket(AF_ALG, SOCK_SEQPACKET, 0);
if (fd2 == -1)
return 2;
struct sockaddr_alg addr = {
.salg_family = AF_ALG,
.salg_type = "hash",
.salg_name = "cryptd(md5-generic)",
};
bind(fd2, &addr, sizeof(addr));

return 0;
}

The backtraces look like:

...
__might_sleep+0x8f/0xe0 kernel/sched/core.c:7260
down_write+0x78/0x180 kernel/locking/rwsem.c:1556
crypto_drop_spawn+0x50/0x220 crypto/algapi.c:709
shash_free_singlespawn_instance+0x19/0x30 crypto/shash.c:621
crypto_shash_free_instance+0x35/0x40 crypto/shash.c:458
crypto_free_instance crypto/algapi.c:68 [inline]
crypto_destroy_instance+0x7d/0xb0 crypto/algapi.c:76
crypto_alg_put crypto/internal.h:108 [inline]
crypto_mod_put crypto/api.c:45 [inline]
crypto_destroy_tfm+0x1f7/0x250 crypto/api.c:573
crypto_free_shash include/crypto/hash.h:734 [inline]
sctp_destruct_common net/sctp/socket.c:5003 [inline]
sctp_v6_destruct_sock+0x40/0x50 net/sctp/socket.c:9436
__sk_destruct+0x56/0x780 net/core/sock.c:1784
sk_destruct net/core/sock.c:1829 [inline]
__sk_free+0x36c/0x470 net/core/sock.c:1840
sk_free+0x51/0x90 net/core/sock.c:1851
sock_put include/net/sock.h:1815 [inline]
sctp_endpoint_destroy_rcu+0xa6/0xf0 net/sctp/endpointola.c:193
rcu_do_batch kernel/rcu/tree.c:2492 [inline]
rcu_core+0x7cc/0x1260 kernel/rcu/tree.c:2733
rcu_core_si+0x9/0x10 kernel/rcu/tree.c:2746
__do_softirq+0x3dc/0x93b kernel/softirq.c:298
...

My analysis is that, when the process dies, the socket freeing is done in a RCU
callback, therefore under softirq context, therefore sleeping is disabled. As
part of freeing a SCTP socket, we free a cryptographical transform that frees a
"spawn" and this grabs a semaphore which triggers this BUG under
CONFIG_DEBUG_ATOMIC_SLEEP=y.

I believe that we could solve this problem by defering any part of this
backtrace into a worker function. Unfortunately, I have no clue about anything
SCTP nor anything crypto/ so I took a stab at defering... something. :) I marked
this as RFC to make it clear I don't hold strong opinions about what should be
defered exactly and expect this will probably change as a result of code review.

I believe that the same bug has been reported by syzbot twice in the past,
without reproducers and once with a slight twist:
- Once, upon freeing a sctp socket (the backtraces are very similar)
https://lore.kernel.org/all/[email protected]/T/
but as far as I can tell the conversation focused on the safety of kfree()
rather than the semaphore protecting the spawns list (maybe I'm missing
something here ?)
- Once, upon freeing a tipc socket:
https://lore.kernel.org/all/[email protected]/T/
Hillf proposed a fix but, as far as I can tell, it didn't get much attention
and focused solely on addressing the bug for tipc sockets.
My fix is inspired by this but further down the backtrace to make the fix work
for both tipc and sctp (and potentially more ?) sockets freeing.

This patch should apply cleanly on v6.5-rc3.

Florent Revest (1):
crypto: Defer transforms destruction to a worker function

crypto/api.c | 26 ++++++++++++++++++--------
include/linux/crypto.h | 3 +++
2 files changed, 21 insertions(+), 8 deletions(-)

--
2.41.0.585.gd2178a4bd4-goog