netdev - Re: [lvc-project] [PATCH] [RFC] net: smc: fix fasync leak in smc

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: <35584a9f-f4c2-423a-8bb8-2c729cedb6fe@yandex.ru>
Date: Wed, 6 Mar 2024 21:07:53 +0300
From: Dmitry Antipov <dmantipov@...dex.ru>
To: Wen Gu <guwen@...ux.alibaba.com>,
 "wenjia@...ux.ibm.com" <wenjia@...ux.ibm.com>
Cc: "lvc-project@...uxtesting.org" <lvc-project@...uxtesting.org>,
 "linux-s390@...r.kernel.org" <linux-s390@...r.kernel.org>,
 "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
 "jaka@...ux.ibm.com" <jaka@...ux.ibm.com>
Subject: Re: [lvc-project] [PATCH] [RFC] net: smc: fix fasync leak in
 smc_release()

On 3/6/24 17:45, Wen Gu wrote:

> IIUC, the fallback (or more precisely the private_data change) essentially
> always happens when the lock_sock(smc->sk) is held, except in smc_listen_work()
> or smc_listen_decline(), but at that moment, userspace program can not yet
> acquire this new socket to add fasync entries to the fasync_list.
> 
> So IMHO, the above patch should work, since it checks the private_data under
> the lock_sock(sk). But if I missed something, please correct me.

Well, the whole picture is somewhat more complicated. Consider the
following diagram (an underlying kernel socket is in [], e.g. [smc->sk]):

Thread 0                        Thread 1

ioctl(sock, FIOASYNC, [1])
...
sock = filp->private_data;
lock_sock(sock [smc->sk]);
sock_fasync(sock, ..., 1)       ; new fasync_struct linked to smc->sk
release_sock(sock [smc->sk]);
                                 ...
                                 lock_sock([smc->sk]);
                                 ...
                                 smc_switch_to_fallback()
                                 ...
                                 smc->clcsock->file->private_data = smc->clcsock;
                                 ...
                                 release_sock([smc->sk]);
ioctl(sock, FIOASYNC, [0])
...
sock = filp->private_data;
lock_sock(sock [smc->clcsock]);
sock_fasync(sock, ..., 0)       ; nothing to unlink from smc->clcsock
                                 ; since fasync entry was linked to smc->sk
release_sock(sock [smc->clcsock]);
                                 ...
                                 close(sock [smc->clcsock]);
                                 __fput(...);
                                 file->f_op->fasync(sock, [0])   ; always failed -
                                                                 ; should use
                                                                 ; smc->sk instead
                                 file->f_op->release()
                                    ...
                                    smc_restore_fallback_changes()
                                    ...
                                    file->private_data = smc->sk.sk_socket;

That is, smc_restore_fallback_changes() restores filp->private_data to
smc->sk. If __fput() would have called file->f_op->release() _before_
file->f_op->fasync(), the fix would be as simple as adding

smc->sk.sk_socket->wq.fasync_list = smc->clcsock->wq.fasync_list;

to smc_restore_fallback_changes(). But since file->f_op->fasync() is called
before file->f_op->release(), the former always makes an attempt to unlink fasync
entry from smc->clcsock instead of smc->sk, thus introducing the memory leak.

And an idea with shared wait queue was intended in attempt to eliminate
this chicken-egg lookalike problem completely.

Dmitry