1. Patchew
  2. linux
  3. View series

[PATCH net v2] scm: fix negative fds with SO_PASSPIDFD

Stas Sergeev posted 1 patch 5 days ago 
include/net/scm.h | 4 ++++
1 file changed, 4 insertions(+)
[PATCH net v2] scm: fix negative fds with SO_PASSPIDFD
Posted by Stas Sergeev 5 days ago 
pidfd_prepare() can return negative values as an error codes.
But scm_pidfd_recv() didn't check for that condition.
As the result, it is possible to create the race that leads to
the negative fds. The race happens if the peer process sends
something to SO_PASSPIDFD-enabled recipient, and quickly exits.
pidfd_prepare() has this code:

    if (!pid || !pid_has_task(pid, thread ? PIDTYPE_PID : PIDTYPE_TGID))
            return -EINVAL;

So if you exit quickly enough, you can hit that EINVAL.
Getting the fd=-22 is very weird, if not exploitable.

This patch adds the missing check and sets MSG_CTRUNC on error.
Recipient can now detect an error by checking this flag.

Changes in v2: add Fixes tag

Signed-off-by: Stas Sergeev <stsp2@yandex.ru>

Fixes: 5e2ff6704a2 ("scm: add SO_PASSPIDFD and SCM_PIDFD")

CC: Alexander Mikhalitsyn <alexander@mihalicyn.com>
CC: "David S. Miller" <davem@davemloft.net>
CC: Eric Dumazet <edumazet@google.com>
CC: Jakub Kicinski <kuba@kernel.org>
CC: Paolo Abeni <pabeni@redhat.com>
CC: Simon Horman <horms@kernel.org>
CC: Christian Brauner <brauner@kernel.org>
CC: Kees Cook <kees@kernel.org>
CC: Kuniyuki Iwashima <kuniyu@amazon.com>
CC: netdev@vger.kernel.org
CC: linux-kernel@vger.kernel.org
---
 include/net/scm.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/net/scm.h b/include/net/scm.h
index 0d35c7c77a74..3ccf8546c506 100644
--- a/include/net/scm.h
+++ b/include/net/scm.h
@@ -155,6 +155,10 @@ static __inline__ void scm_pidfd_recv(struct msghdr *msg, struct scm_cookie *scm
 		return;
 
 	pidfd = pidfd_prepare(scm->pid, 0, &pidfd_file);
+	if (pidfd < 0) {
+		msg->msg_flags |= MSG_CTRUNC;
+		return;
+	}
 
 	if (put_cmsg(msg, SOL_SOCKET, SCM_PIDFD, sizeof(int), &pidfd)) {
 		if (pidfd_file) {
-- 
2.47.0
Re: [PATCH net v2] scm: fix negative fds with SO_PASSPIDFD
Posted by Alexander Mikhalitsyn 5 days ago 
Am So., 17. Nov. 2024 um 10:13 Uhr schrieb Stas Sergeev <stsp2@yandex.ru>:
>
> pidfd_prepare() can return negative values as an error codes.
> But scm_pidfd_recv() didn't check for that condition.
> As the result, it is possible to create the race that leads to
> the negative fds. The race happens if the peer process sends
> something to SO_PASSPIDFD-enabled recipient, and quickly exits.
> pidfd_prepare() has this code:
>
>     if (!pid || !pid_has_task(pid, thread ? PIDTYPE_PID : PIDTYPE_TGID))
>             return -EINVAL;
>
> So if you exit quickly enough, you can hit that EINVAL.
> Getting the fd=-22 is very weird, if not exploitable.
>
> This patch adds the missing check and sets MSG_CTRUNC on error.
> Recipient can now detect an error by checking this flag.
>
> Changes in v2: add Fixes tag
>
> Signed-off-by: Stas Sergeev <stsp2@yandex.ru>

Hi Stas,

Actually, it's not a forgotten check. It's intended behavior to pass
through errors from pidfd_prepare() to
the userspace. In my first version [1] of the patch I tried to return
ESRCH instead of EINVAL in your case, but
then during discussions we decided to remove that.

[1] https://lore.kernel.org/all/20230316131526.283569-2-aleksandr.mikhalitsyn@canonical.com/

Kind regards,
Alex

>
> Fixes: 5e2ff6704a2 ("scm: add SO_PASSPIDFD and SCM_PIDFD")
>
> CC: Alexander Mikhalitsyn <alexander@mihalicyn.com>
> CC: "David S. Miller" <davem@davemloft.net>
> CC: Eric Dumazet <edumazet@google.com>
> CC: Jakub Kicinski <kuba@kernel.org>
> CC: Paolo Abeni <pabeni@redhat.com>
> CC: Simon Horman <horms@kernel.org>
> CC: Christian Brauner <brauner@kernel.org>
> CC: Kees Cook <kees@kernel.org>
> CC: Kuniyuki Iwashima <kuniyu@amazon.com>
> CC: netdev@vger.kernel.org
> CC: linux-kernel@vger.kernel.org
> ---
>  include/net/scm.h | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/include/net/scm.h b/include/net/scm.h
> index 0d35c7c77a74..3ccf8546c506 100644
> --- a/include/net/scm.h
> +++ b/include/net/scm.h
> @@ -155,6 +155,10 @@ static __inline__ void scm_pidfd_recv(struct msghdr *msg, struct scm_cookie *scm
>                 return;
>
>         pidfd = pidfd_prepare(scm->pid, 0, &pidfd_file);
> +       if (pidfd < 0) {
> +               msg->msg_flags |= MSG_CTRUNC;
> +               return;
> +       }
>
>         if (put_cmsg(msg, SOL_SOCKET, SCM_PIDFD, sizeof(int), &pidfd)) {
>                 if (pidfd_file) {
> --
> 2.47.0
>
Re: [PATCH net v2] scm: fix negative fds with SO_PASSPIDFD
Posted by stsp 5 days ago 
17.11.2024 12:40, Alexander Mikhalitsyn пишет:
> Hi Stas,
>
> Actually, it's not a forgotten check. It's intended behavior to pass
> through errors from pidfd_prepare() to
> the userspace. In my first version [1] of the patch I tried to return
> ESRCH instead of EINVAL in your case, but
> then during discussions we decided to remove that.
>
> [1] https://lore.kernel.org/all/20230316131526.283569-2-aleksandr.mikhalitsyn@canonical.com/
Yes, the patch you referenced above,
only calls put_cmsg() with an error code.

But the code I can see now in git, does
much more. Namely,
if (pidfd_file)
     fd_install(pidfd, pidfd_file);

Or:

put_unused_fd(pidfd);

And I really can't find any ">=0" check
in those funcs. What am I missing?
Is it safe to call fd_install(-22, pidfd_file)?
Re: [PATCH net v2] scm: fix negative fds with SO_PASSPIDFD
Posted by stsp 4 days, 23 hours ago 
17.11.2024 13:04, stsp пишет:
> 17.11.2024 12:40, Alexander Mikhalitsyn пишет:
>> Hi Stas,
>>
>> Actually, it's not a forgotten check. It's intended behavior to pass
>> through errors from pidfd_prepare() to
>> the userspace. In my first version [1] of the patch I tried to return
>> ESRCH instead of EINVAL in your case, but
>> then during discussions we decided to remove that.
>>
>> [1] 
>> https://lore.kernel.org/all/20230316131526.283569-2-aleksandr.mikhalitsyn@canonical.com/
> Yes, the patch you referenced above,
> only calls put_cmsg() with an error code.
>
> But the code I can see now in git, does
> much more. Namely,
> if (pidfd_file)
>     fd_install(pidfd, pidfd_file); 
Ah, I guess pidfd_file is a culprit.
Thanks.
Re: [PATCH net v2] scm: fix negative fds with SO_PASSPIDFD
Posted by Alexander Mikhalitsyn 3 days, 19 hours ago 
Am So., 17. Nov. 2024 um 11:11 Uhr schrieb stsp <stsp2@yandex.ru>:
>
> 17.11.2024 13:04, stsp пишет:
> > 17.11.2024 12:40, Alexander Mikhalitsyn пишет:
> >> Hi Stas,
> >>
> >> Actually, it's not a forgotten check. It's intended behavior to pass
> >> through errors from pidfd_prepare() to
> >> the userspace. In my first version [1] of the patch I tried to return
> >> ESRCH instead of EINVAL in your case, but
> >> then during discussions we decided to remove that.
> >>
> >> [1]
> >> https://lore.kernel.org/all/20230316131526.283569-2-aleksandr.mikhalitsyn@canonical.com/
> > Yes, the patch you referenced above,
> > only calls put_cmsg() with an error code.
> >
> > But the code I can see now in git, does
> > much more. Namely,
> > if (pidfd_file)
> >     fd_install(pidfd, pidfd_file);
> Ah, I guess pidfd_file is a culprit.

Hey,

Precisely, when an error happens then pidfd_file is NULL.

Kind regards,
Alex

> Thanks.

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.