From: Rodrigo Campos <rodrigo@kinvolk.io> To: Kees Cook <keescook@chromium.org>, Andy Lutomirski <luto@amacapital.net>, Will Drewry <wad@chromium.org>, linux-kernel@vger.kernel.org, containers@lists.linux-foundation.org Cc: Alban Crequy <alban@kinvolk.io>, stable@vger.kernel.org Subject: [PATCH 1/1] seccomp: Always "goto wait" if the list is empty Date: Tue, 13 Apr 2021 18:01:51 +0200 [thread overview] Message-ID: <20210413160151.3301-2-rodrigo@kinvolk.io> (raw) In-Reply-To: <20210413160151.3301-1-rodrigo@kinvolk.io> It is possible for the thread with the seccomp filter attached (target) to be waken up by an addfd message, but the list be empty. This happens when the addfd ioctl on the other side (seccomp agent) is interrupted by a signal such as SIGURG. In that case, the target erroneously and prematurely returns from the syscall to userspace even though the seccomp agent didn't ask for it. This happens in the following scenario: seccomp_notify_addfd() | seccomp_do_user_notification() | | err = wait_for_completion_interruptible(&n.ready); complete(&knotif->ready); | ret = wait_for_completion_interruptible(&kaddfd.completion); | // interrupted | | mutex_lock(&filter->notify_lock); | list_del(&kaddfd.list); | mutex_unlock(&filter->notify_lock); | | mutex_lock(&match->notify_lock); | // This is false, addfd is false | if (addfd && n.state != SECCOMP_NOTIFY_REPLIED) | | ret = n.val; | err = n.error; | flags = n.flags; So, the process blocked in seccomp_do_user_notification() will see a response. As n is 0 initialized and wasn't set, it will see a 0 as return value from the syscall. The seccomp agent, when retrying the interrupted syscall, will see an ENOENT error as the notification no longer exists (it was already answered by this bug). This patch fixes the issue by splitting the if in two parts: if we were woken up and the state is not replied, we will always do a "goto wait". And if that happens and there is an addfd element on the list, we will add the fd before "goto wait". This issue is present since 5.9, when addfd was added. Fixes: 7cf97b1254550 Cc: stable@vger.kernel.org # 5.9+ Signed-off-by: Rodrigo Campos <rodrigo@kinvolk.io> --- kernel/seccomp.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/kernel/seccomp.c b/kernel/seccomp.c index 63b40d12896b..1b34598f0e07 100644 --- a/kernel/seccomp.c +++ b/kernel/seccomp.c @@ -1107,11 +1107,20 @@ static int seccomp_do_user_notification(int this_syscall, err = wait_for_completion_interruptible(&n.ready); mutex_lock(&match->notify_lock); if (err == 0) { - /* Check if we were woken up by a addfd message */ - addfd = list_first_entry_or_null(&n.addfd, - struct seccomp_kaddfd, list); - if (addfd && n.state != SECCOMP_NOTIFY_REPLIED) { - seccomp_handle_addfd(addfd); + + if (n.state != SECCOMP_NOTIFY_REPLIED) { + /* + * It is possible to be waken-up by an addfd message but + * the list be empty. This can happen if the addfd + * ioctl() is interrupted, as it deletes the element. + * + * So, check if indeed there is an element in the list. + */ + addfd = list_first_entry_or_null(&n.addfd, + struct seccomp_kaddfd, list); + if (addfd) + seccomp_handle_addfd(addfd); + mutex_unlock(&match->notify_lock); goto wait; } -- 2.30.2 _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/containers
WARNING: multiple messages have this Message-ID (diff)
From: Rodrigo Campos <rodrigo@kinvolk.io> To: Kees Cook <keescook@chromium.org>, Andy Lutomirski <luto@amacapital.net>, Will Drewry <wad@chromium.org>, linux-kernel@vger.kernel.org, containers@lists.linux-foundation.org Cc: "Rodrigo Campos" <rodrigo@kinvolk.io>, "Sargun Dhillon" <sargun@sargun.me>, "Mauricio Vásquez Bernal" <mauricio@kinvolk.io>, "Alban Crequy" <alban@kinvolk.io>, stable@vger.kernel.org Subject: [PATCH 1/1] seccomp: Always "goto wait" if the list is empty Date: Tue, 13 Apr 2021 18:01:51 +0200 [thread overview] Message-ID: <20210413160151.3301-2-rodrigo@kinvolk.io> (raw) In-Reply-To: <20210413160151.3301-1-rodrigo@kinvolk.io> It is possible for the thread with the seccomp filter attached (target) to be waken up by an addfd message, but the list be empty. This happens when the addfd ioctl on the other side (seccomp agent) is interrupted by a signal such as SIGURG. In that case, the target erroneously and prematurely returns from the syscall to userspace even though the seccomp agent didn't ask for it. This happens in the following scenario: seccomp_notify_addfd() | seccomp_do_user_notification() | | err = wait_for_completion_interruptible(&n.ready); complete(&knotif->ready); | ret = wait_for_completion_interruptible(&kaddfd.completion); | // interrupted | | mutex_lock(&filter->notify_lock); | list_del(&kaddfd.list); | mutex_unlock(&filter->notify_lock); | | mutex_lock(&match->notify_lock); | // This is false, addfd is false | if (addfd && n.state != SECCOMP_NOTIFY_REPLIED) | | ret = n.val; | err = n.error; | flags = n.flags; So, the process blocked in seccomp_do_user_notification() will see a response. As n is 0 initialized and wasn't set, it will see a 0 as return value from the syscall. The seccomp agent, when retrying the interrupted syscall, will see an ENOENT error as the notification no longer exists (it was already answered by this bug). This patch fixes the issue by splitting the if in two parts: if we were woken up and the state is not replied, we will always do a "goto wait". And if that happens and there is an addfd element on the list, we will add the fd before "goto wait". This issue is present since 5.9, when addfd was added. Fixes: 7cf97b1254550 Cc: stable@vger.kernel.org # 5.9+ Signed-off-by: Rodrigo Campos <rodrigo@kinvolk.io> --- kernel/seccomp.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/kernel/seccomp.c b/kernel/seccomp.c index 63b40d12896b..1b34598f0e07 100644 --- a/kernel/seccomp.c +++ b/kernel/seccomp.c @@ -1107,11 +1107,20 @@ static int seccomp_do_user_notification(int this_syscall, err = wait_for_completion_interruptible(&n.ready); mutex_lock(&match->notify_lock); if (err == 0) { - /* Check if we were woken up by a addfd message */ - addfd = list_first_entry_or_null(&n.addfd, - struct seccomp_kaddfd, list); - if (addfd && n.state != SECCOMP_NOTIFY_REPLIED) { - seccomp_handle_addfd(addfd); + + if (n.state != SECCOMP_NOTIFY_REPLIED) { + /* + * It is possible to be waken-up by an addfd message but + * the list be empty. This can happen if the addfd + * ioctl() is interrupted, as it deletes the element. + * + * So, check if indeed there is an element in the list. + */ + addfd = list_first_entry_or_null(&n.addfd, + struct seccomp_kaddfd, list); + if (addfd) + seccomp_handle_addfd(addfd); + mutex_unlock(&match->notify_lock); goto wait; } -- 2.30.2
next prev parent reply other threads:[~2021-04-13 16:02 UTC|newest] Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-04-13 16:01 [PATCH 0/1] seccomp: Erroneous return on interrupted addfd ioctl() Rodrigo Campos 2021-04-13 16:01 ` Rodrigo Campos 2021-04-13 16:01 ` Rodrigo Campos [this message] 2021-04-13 16:01 ` [PATCH 1/1] seccomp: Always "goto wait" if the list is empty Rodrigo Campos 2021-04-13 17:53 ` Christian Brauner 2021-04-13 17:53 ` Christian Brauner 2021-04-13 18:02 ` Rodrigo Campos 2021-04-13 18:02 ` Rodrigo Campos
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210413160151.3301-2-rodrigo@kinvolk.io \ --to=rodrigo@kinvolk.io \ --cc=alban@kinvolk.io \ --cc=containers@lists.linux-foundation.org \ --cc=keescook@chromium.org \ --cc=linux-kernel@vger.kernel.org \ --cc=luto@amacapital.net \ --cc=stable@vger.kernel.org \ --cc=wad@chromium.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.