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: <93077cee-b81a-4690-9aa8-cc954f9be902@linux.ibm.com>
Date: Thu, 7 Mar 2024 10:57:40 +0100
From: Jan Karcher <jaka@...ux.ibm.com>
To: Dmitry Antipov <dmantipov@...dex.ru>, Wen Gu <guwen@...ux.alibaba.com>,
        "wenjia@...ux.ibm.com" <wenjia@...ux.ibm.com>,
        Gerd Bayer <gbayer@...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>
Subject: Re: [lvc-project] [PATCH] [RFC] net: smc: fix fasync leak in
 smc_release()



On 06/03/2024 19:07, Dmitry Antipov wrote:
> 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
> 

Me and Gerd had another look at this.
The infrastructure for what i proposed in the last E-Mail regarding the 
ioctl function handler is already there (af_smc.c#smc_ioctl).
There we already check if we are in a active fallback to send the ioctls 
to the clcsock instead of the sk socket.

```
	lock_sock(&smc->sk);
	if (smc->use_fallback) {
		if (!smc->clcsock) {
			release_sock(&smc->sk);
			return -EBADF;
		}
		answ = smc->clcsock->ops->ioctl(smc->clcsock, cmd, arg);
		release_sock(&smc->sk);
		return answ;
	}
```

We think it might be an option to secure the path in this function with 
the smc->clcsock_release_lock.

```
	lock_sock(&smc->sk);
	if (smc->use_fallback) {
		if (!smc->clcsock) {
			release_sock(&smc->sk);
			return -EBADF;
		}
+		mutex_lock(&smc->clcsock_release_lock);
		answ = smc->clcsock->ops->ioctl(smc->clcsock, cmd, arg);
+		mutex_unlock(&smc->clcsock_release_lock);
		release_sock(&smc->sk);
		return answ;
	}
```

What do yo think about this?
I'm going to test this idea and see if we canget rid of the leak this way.

Thanks
- Jan & Gerd