netdev - Re: [PATCH net v2] scm: fix negative fds with SO

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: <CAJqdLrp4J57k67R3OWM-_6QZSv8EV9UANzdAtBCiLGQZPTXDcQ@mail.gmail.com>
Date: Sun, 17 Nov 2024 10:40:48 +0100
From: Alexander Mikhalitsyn <alexander@...alicyn.com>
To: Stas Sergeev <stsp2@...dex.ru>
Cc: linux-kernel@...r.kernel.org, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Simon Horman <horms@...nel.org>, Christian Brauner <brauner@...nel.org>, Kees Cook <kees@...nel.org>, 
	Kuniyuki Iwashima <kuniyu@...zon.com>, netdev@...r.kernel.org
Subject: Re: [PATCH net v2] scm: fix negative fds with SO_PASSPIDFD

Am So., 17. Nov. 2024 um 10:13 Uhr schrieb Stas Sergeev <stsp2@...dex.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@...dex.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@...alicyn.com>
> CC: "David S. Miller" <davem@...emloft.net>
> CC: Eric Dumazet <edumazet@...gle.com>
> CC: Jakub Kicinski <kuba@...nel.org>
> CC: Paolo Abeni <pabeni@...hat.com>
> CC: Simon Horman <horms@...nel.org>
> CC: Christian Brauner <brauner@...nel.org>
> CC: Kees Cook <kees@...nel.org>
> CC: Kuniyuki Iwashima <kuniyu@...zon.com>
> CC: netdev@...r.kernel.org
> CC: linux-kernel@...r.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
>