* [PATCH v3] seccomp: Add find_notification helper
@ 2020-06-01 11:25 Sargun Dhillon
2020-06-01 18:26 ` Kees Cook
2020-06-17 20:08 ` Nathan Chancellor
0 siblings, 2 replies; 6+ messages in thread
From: Sargun Dhillon @ 2020-06-01 11:25 UTC (permalink / raw)
To: containers, keescook, linux-api, linux-kernel
Cc: Sargun Dhillon, viro, christian.brauner, cyphar, jannh, jeffv,
palmer, rsesek, tycho, Matt Denton, Kees Cook
This adds a helper which can iterate through a seccomp_filter to
find a notification matching an ID. It removes several replicated
chunks of code.
Signed-off-by: Sargun Dhillon <sargun@sargun.me>
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
Reviewed-by: Tycho Andersen <tycho@tycho.ws>
Cc: Matt Denton <mpdenton@google.com>
Cc: Kees Cook <keescook@google.com>,
Cc: Jann Horn <jannh@google.com>,
Cc: Robert Sesek <rsesek@google.com>,
Cc: Chris Palmer <palmer@google.com>
Cc: Christian Brauner <christian.brauner@ubuntu.com>
Cc: Tycho Andersen <tycho@tycho.ws>
---
kernel/seccomp.c | 55 ++++++++++++++++++++++++------------------------
1 file changed, 28 insertions(+), 27 deletions(-)
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 55a6184f5990..cc6b47173a95 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -41,6 +41,7 @@
#include <linux/tracehook.h>
#include <linux/uaccess.h>
#include <linux/anon_inodes.h>
+#include <linux/lockdep.h>
enum notify_state {
SECCOMP_NOTIFY_INIT,
@@ -1021,10 +1022,27 @@ static int seccomp_notify_release(struct inode *inode, struct file *file)
return 0;
}
+/* must be called with notif_lock held */
+static inline struct seccomp_knotif *
+find_notification(struct seccomp_filter *filter, u64 id)
+{
+ struct seccomp_knotif *cur;
+
+ lockdep_assert_held(&filter->notify_lock);
+
+ list_for_each_entry(cur, &filter->notif->notifications, list) {
+ if (cur->id == id)
+ return cur;
+ }
+
+ return NULL;
+}
+
+
static long seccomp_notify_recv(struct seccomp_filter *filter,
void __user *buf)
{
- struct seccomp_knotif *knotif = NULL, *cur;
+ struct seccomp_knotif *knotif, *cur;
struct seccomp_notif unotif;
ssize_t ret;
@@ -1078,15 +1096,8 @@ static long seccomp_notify_recv(struct seccomp_filter *filter,
* may have died when we released the lock, so we need to make
* sure it's still around.
*/
- knotif = NULL;
mutex_lock(&filter->notify_lock);
- list_for_each_entry(cur, &filter->notif->notifications, list) {
- if (cur->id == unotif.id) {
- knotif = cur;
- break;
- }
- }
-
+ knotif = find_notification(filter, unotif.id);
if (knotif) {
knotif->state = SECCOMP_NOTIFY_INIT;
up(&filter->notif->request);
@@ -1101,7 +1112,7 @@ static long seccomp_notify_send(struct seccomp_filter *filter,
void __user *buf)
{
struct seccomp_notif_resp resp = {};
- struct seccomp_knotif *knotif = NULL, *cur;
+ struct seccomp_knotif *knotif;
long ret;
if (copy_from_user(&resp, buf, sizeof(resp)))
@@ -1118,13 +1129,7 @@ static long seccomp_notify_send(struct seccomp_filter *filter,
if (ret < 0)
return ret;
- list_for_each_entry(cur, &filter->notif->notifications, list) {
- if (cur->id == resp.id) {
- knotif = cur;
- break;
- }
- }
-
+ knotif = find_notification(filter, resp.id);
if (!knotif) {
ret = -ENOENT;
goto out;
@@ -1150,7 +1155,7 @@ static long seccomp_notify_send(struct seccomp_filter *filter,
static long seccomp_notify_id_valid(struct seccomp_filter *filter,
void __user *buf)
{
- struct seccomp_knotif *knotif = NULL;
+ struct seccomp_knotif *knotif;
u64 id;
long ret;
@@ -1161,16 +1166,12 @@ static long seccomp_notify_id_valid(struct seccomp_filter *filter,
if (ret < 0)
return ret;
- ret = -ENOENT;
- list_for_each_entry(knotif, &filter->notif->notifications, list) {
- if (knotif->id == id) {
- if (knotif->state == SECCOMP_NOTIFY_SENT)
- ret = 0;
- goto out;
- }
- }
+ knotif = find_notification(filter, id);
+ if (knotif && knotif->state == SECCOMP_NOTIFY_SENT)
+ ret = 0;
+ else
+ ret = -ENOENT;
-out:
mutex_unlock(&filter->notify_lock);
return ret;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3] seccomp: Add find_notification helper
2020-06-01 11:25 [PATCH v3] seccomp: Add find_notification helper Sargun Dhillon
@ 2020-06-01 18:26 ` Kees Cook
2020-06-17 20:08 ` Nathan Chancellor
1 sibling, 0 replies; 6+ messages in thread
From: Kees Cook @ 2020-06-01 18:26 UTC (permalink / raw)
To: Sargun Dhillon
Cc: containers, linux-api, linux-kernel, viro, christian.brauner,
cyphar, jannh, jeffv, palmer, rsesek, tycho, Matt Denton
On Mon, Jun 01, 2020 at 04:25:32AM -0700, Sargun Dhillon wrote:
> This adds a helper which can iterate through a seccomp_filter to
> find a notification matching an ID. It removes several replicated
> chunks of code.
>
> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
> Reviewed-by: Tycho Andersen <tycho@tycho.ws>
> Cc: Matt Denton <mpdenton@google.com>
> Cc: Kees Cook <keescook@google.com>,
> Cc: Jann Horn <jannh@google.com>,
> Cc: Robert Sesek <rsesek@google.com>,
> Cc: Chris Palmer <palmer@google.com>
> Cc: Christian Brauner <christian.brauner@ubuntu.com>
> Cc: Tycho Andersen <tycho@tycho.ws>
Thanks! Applied to for-next/seccomp.
--
Kees Cook
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] seccomp: Add find_notification helper
2020-06-01 11:25 [PATCH v3] seccomp: Add find_notification helper Sargun Dhillon
2020-06-01 18:26 ` Kees Cook
@ 2020-06-17 20:08 ` Nathan Chancellor
2020-06-17 21:22 ` Kees Cook
2020-06-18 5:44 ` Sargun Dhillon
1 sibling, 2 replies; 6+ messages in thread
From: Nathan Chancellor @ 2020-06-17 20:08 UTC (permalink / raw)
To: Sargun Dhillon
Cc: containers, keescook, linux-api, linux-kernel, viro,
christian.brauner, cyphar, jannh, jeffv, palmer, rsesek, tycho,
Matt Denton, Kees Cook
On Mon, Jun 01, 2020 at 04:25:32AM -0700, Sargun Dhillon wrote:
> This adds a helper which can iterate through a seccomp_filter to
> find a notification matching an ID. It removes several replicated
> chunks of code.
>
> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
> Reviewed-by: Tycho Andersen <tycho@tycho.ws>
> Cc: Matt Denton <mpdenton@google.com>
> Cc: Kees Cook <keescook@google.com>,
> Cc: Jann Horn <jannh@google.com>,
> Cc: Robert Sesek <rsesek@google.com>,
> Cc: Chris Palmer <palmer@google.com>
> Cc: Christian Brauner <christian.brauner@ubuntu.com>
> Cc: Tycho Andersen <tycho@tycho.ws>
> ---
> kernel/seccomp.c | 55 ++++++++++++++++++++++++------------------------
> 1 file changed, 28 insertions(+), 27 deletions(-)
>
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 55a6184f5990..cc6b47173a95 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -41,6 +41,7 @@
> #include <linux/tracehook.h>
> #include <linux/uaccess.h>
> #include <linux/anon_inodes.h>
> +#include <linux/lockdep.h>
>
> enum notify_state {
> SECCOMP_NOTIFY_INIT,
> @@ -1021,10 +1022,27 @@ static int seccomp_notify_release(struct inode *inode, struct file *file)
> return 0;
> }
>
> +/* must be called with notif_lock held */
> +static inline struct seccomp_knotif *
> +find_notification(struct seccomp_filter *filter, u64 id)
> +{
> + struct seccomp_knotif *cur;
> +
> + lockdep_assert_held(&filter->notify_lock);
> +
> + list_for_each_entry(cur, &filter->notif->notifications, list) {
> + if (cur->id == id)
> + return cur;
> + }
> +
> + return NULL;
> +}
> +
> +
> static long seccomp_notify_recv(struct seccomp_filter *filter,
> void __user *buf)
> {
> - struct seccomp_knotif *knotif = NULL, *cur;
> + struct seccomp_knotif *knotif, *cur;
> struct seccomp_notif unotif;
> ssize_t ret;
>
> @@ -1078,15 +1096,8 @@ static long seccomp_notify_recv(struct seccomp_filter *filter,
> * may have died when we released the lock, so we need to make
> * sure it's still around.
> */
> - knotif = NULL;
> mutex_lock(&filter->notify_lock);
> - list_for_each_entry(cur, &filter->notif->notifications, list) {
> - if (cur->id == unotif.id) {
> - knotif = cur;
> - break;
> - }
> - }
> -
> + knotif = find_notification(filter, unotif.id);
> if (knotif) {
> knotif->state = SECCOMP_NOTIFY_INIT;
> up(&filter->notif->request);
> @@ -1101,7 +1112,7 @@ static long seccomp_notify_send(struct seccomp_filter *filter,
> void __user *buf)
> {
> struct seccomp_notif_resp resp = {};
> - struct seccomp_knotif *knotif = NULL, *cur;
> + struct seccomp_knotif *knotif;
> long ret;
>
> if (copy_from_user(&resp, buf, sizeof(resp)))
> @@ -1118,13 +1129,7 @@ static long seccomp_notify_send(struct seccomp_filter *filter,
> if (ret < 0)
> return ret;
>
> - list_for_each_entry(cur, &filter->notif->notifications, list) {
> - if (cur->id == resp.id) {
> - knotif = cur;
> - break;
> - }
> - }
> -
> + knotif = find_notification(filter, resp.id);
> if (!knotif) {
> ret = -ENOENT;
> goto out;
> @@ -1150,7 +1155,7 @@ static long seccomp_notify_send(struct seccomp_filter *filter,
> static long seccomp_notify_id_valid(struct seccomp_filter *filter,
> void __user *buf)
> {
> - struct seccomp_knotif *knotif = NULL;
I don't know that this should have been removed, clang now warns:
kernel/seccomp.c:1063:2: warning: variable 'knotif' is used uninitialized whenever 'for' loop exits because its condition is false [-Wsometimes-uninitialized]
list_for_each_entry(cur, &filter->notif->notifications, list) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/list.h:602:7: note: expanded from macro 'list_for_each_entry'
&pos->member != (head); \
^~~~~~~~~~~~~~~~~~~~~~
kernel/seccomp.c:1075:7: note: uninitialized use occurs here
if (!knotif) {
^~~~~~
kernel/seccomp.c:1063:2: note: remove the condition if it is always true
list_for_each_entry(cur, &filter->notif->notifications, list) {
^
include/linux/list.h:602:7: note: expanded from macro 'list_for_each_entry'
&pos->member != (head); \
^
kernel/seccomp.c:1045:31: note: initialize the variable 'knotif' to silence this warning
struct seccomp_knotif *knotif, *cur;
^
= NULL
1 warning generated.
> + struct seccomp_knotif *knotif;
> u64 id;
> long ret;
>
> @@ -1161,16 +1166,12 @@ static long seccomp_notify_id_valid(struct seccomp_filter *filter,
> if (ret < 0)
> return ret;
>
> - ret = -ENOENT;
> - list_for_each_entry(knotif, &filter->notif->notifications, list) {
> - if (knotif->id == id) {
> - if (knotif->state == SECCOMP_NOTIFY_SENT)
> - ret = 0;
> - goto out;
> - }
> - }
> + knotif = find_notification(filter, id);
> + if (knotif && knotif->state == SECCOMP_NOTIFY_SENT)
> + ret = 0;
> + else
> + ret = -ENOENT;
>
> -out:
> mutex_unlock(&filter->notify_lock);
> return ret;
> }
> --
> 2.25.1
>
Cheers,
Nathan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] seccomp: Add find_notification helper
2020-06-17 20:08 ` Nathan Chancellor
@ 2020-06-17 21:22 ` Kees Cook
2020-06-18 5:44 ` Sargun Dhillon
1 sibling, 0 replies; 6+ messages in thread
From: Kees Cook @ 2020-06-17 21:22 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Sargun Dhillon, containers, linux-api, linux-kernel, viro,
christian.brauner, cyphar, jannh, jeffv, palmer, rsesek, tycho,
Matt Denton
On Wed, Jun 17, 2020 at 01:08:44PM -0700, Nathan Chancellor wrote:
> On Mon, Jun 01, 2020 at 04:25:32AM -0700, Sargun Dhillon wrote:
> > [...]
> > static long seccomp_notify_recv(struct seccomp_filter *filter,
> > void __user *buf)
> > {
> > - struct seccomp_knotif *knotif = NULL, *cur;
> > + struct seccomp_knotif *knotif, *cur;
> > struct seccomp_notif unotif;
> > ssize_t ret;
> >
>
> I don't know that this should have been removed, clang now warns:
>
> kernel/seccomp.c:1063:2: warning: variable 'knotif' is used uninitialized whenever 'for' loop exits because its condition is false [-Wsometimes-uninitialized]
> list_for_each_entry(cur, &filter->notif->notifications, list) {
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> include/linux/list.h:602:7: note: expanded from macro 'list_for_each_entry'
> &pos->member != (head); \
> ^~~~~~~~~~~~~~~~~~~~~~
> kernel/seccomp.c:1075:7: note: uninitialized use occurs here
> if (!knotif) {
> ^~~~~~
> kernel/seccomp.c:1063:2: note: remove the condition if it is always true
> list_for_each_entry(cur, &filter->notif->notifications, list) {
> ^
> include/linux/list.h:602:7: note: expanded from macro 'list_for_each_entry'
> &pos->member != (head); \
> ^
> kernel/seccomp.c:1045:31: note: initialize the variable 'knotif' to silence this warning
> struct seccomp_knotif *knotif, *cur;
> ^
> = NULL
> 1 warning generated.
Eek; yes, thank you! I've folded the fix into Sargun's patch.
--
Kees Cook
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] seccomp: Add find_notification helper
2020-06-17 20:08 ` Nathan Chancellor
2020-06-17 21:22 ` Kees Cook
@ 2020-06-18 5:44 ` Sargun Dhillon
2020-06-18 6:04 ` Nathan Chancellor
1 sibling, 1 reply; 6+ messages in thread
From: Sargun Dhillon @ 2020-06-18 5:44 UTC (permalink / raw)
To: Nathan Chancellor
Cc: containers, keescook, linux-api, linux-kernel, viro,
christian.brauner, cyphar, jannh, jeffv, palmer, rsesek, tycho,
Matt Denton, Kees Cook
On Wed, Jun 17, 2020 at 01:08:44PM -0700, Nathan Chancellor wrote:
> On Mon, Jun 01, 2020 at 04:25:32AM -0700, Sargun Dhillon wrote:
> > This adds a helper which can iterate through a seccomp_filter to
> > find a notification matching an ID. It removes several replicated
> > chunks of code.
> >
> > Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> > Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
> > Reviewed-by: Tycho Andersen <tycho@tycho.ws>
> > Cc: Matt Denton <mpdenton@google.com>
> > Cc: Kees Cook <keescook@google.com>,
> > Cc: Jann Horn <jannh@google.com>,
> > Cc: Robert Sesek <rsesek@google.com>,
> > Cc: Chris Palmer <palmer@google.com>
> > Cc: Christian Brauner <christian.brauner@ubuntu.com>
> > Cc: Tycho Andersen <tycho@tycho.ws>
> > ---
> > kernel/seccomp.c | 55 ++++++++++++++++++++++++------------------------
> > 1 file changed, 28 insertions(+), 27 deletions(-)
> >
> > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > index 55a6184f5990..cc6b47173a95 100644
> > --- a/kernel/seccomp.c
> > +++ b/kernel/seccomp.c
> > @@ -41,6 +41,7 @@
> > #include <linux/tracehook.h>
> > #include <linux/uaccess.h>
> > #include <linux/anon_inodes.h>
> > +#include <linux/lockdep.h>
> >
> > enum notify_state {
> > SECCOMP_NOTIFY_INIT,
> > @@ -1021,10 +1022,27 @@ static int seccomp_notify_release(struct inode *inode, struct file *file)
> > return 0;
> > }
> >
> > +/* must be called with notif_lock held */
> > +static inline struct seccomp_knotif *
> > +find_notification(struct seccomp_filter *filter, u64 id)
> > +{
> > + struct seccomp_knotif *cur;
> > +
> > + lockdep_assert_held(&filter->notify_lock);
> > +
> > + list_for_each_entry(cur, &filter->notif->notifications, list) {
> > + if (cur->id == id)
> > + return cur;
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > +
> > static long seccomp_notify_recv(struct seccomp_filter *filter,
> > void __user *buf)
> > {
> > - struct seccomp_knotif *knotif = NULL, *cur;
> > + struct seccomp_knotif *knotif, *cur;
> > struct seccomp_notif unotif;
> > ssize_t ret;
> >
> > @@ -1078,15 +1096,8 @@ static long seccomp_notify_recv(struct seccomp_filter *filter,
> > * may have died when we released the lock, so we need to make
> > * sure it's still around.
> > */
> > - knotif = NULL;
> > mutex_lock(&filter->notify_lock);
> > - list_for_each_entry(cur, &filter->notif->notifications, list) {
> > - if (cur->id == unotif.id) {
> > - knotif = cur;
> > - break;
> > - }
> > - }
> > -
> > + knotif = find_notification(filter, unotif.id);
> > if (knotif) {
> > knotif->state = SECCOMP_NOTIFY_INIT;
> > up(&filter->notif->request);
> > @@ -1101,7 +1112,7 @@ static long seccomp_notify_send(struct seccomp_filter *filter,
> > void __user *buf)
> > {
> > struct seccomp_notif_resp resp = {};
> > - struct seccomp_knotif *knotif = NULL, *cur;
> > + struct seccomp_knotif *knotif;
> > long ret;
> >
> > if (copy_from_user(&resp, buf, sizeof(resp)))
> > @@ -1118,13 +1129,7 @@ static long seccomp_notify_send(struct seccomp_filter *filter,
> > if (ret < 0)
> > return ret;
> >
> > - list_for_each_entry(cur, &filter->notif->notifications, list) {
> > - if (cur->id == resp.id) {
> > - knotif = cur;
> > - break;
> > - }
> > - }
> > -
> > + knotif = find_notification(filter, resp.id);
> > if (!knotif) {
> > ret = -ENOENT;
> > goto out;
> > @@ -1150,7 +1155,7 @@ static long seccomp_notify_send(struct seccomp_filter *filter,
> > static long seccomp_notify_id_valid(struct seccomp_filter *filter,
> > void __user *buf)
> > {
> > - struct seccomp_knotif *knotif = NULL;
>
> I don't know that this should have been removed, clang now warns:
>
> kernel/seccomp.c:1063:2: warning: variable 'knotif' is used uninitialized whenever 'for' loop exits because its condition is false [-Wsometimes-uninitialized]
> list_for_each_entry(cur, &filter->notif->notifications, list) {
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> include/linux/list.h:602:7: note: expanded from macro 'list_for_each_entry'
> &pos->member != (head); \
> ^~~~~~~~~~~~~~~~~~~~~~
> kernel/seccomp.c:1075:7: note: uninitialized use occurs here
> if (!knotif) {
> ^~~~~~
> kernel/seccomp.c:1063:2: note: remove the condition if it is always true
> list_for_each_entry(cur, &filter->notif->notifications, list) {
> ^
> include/linux/list.h:602:7: note: expanded from macro 'list_for_each_entry'
> &pos->member != (head); \
> ^
> kernel/seccomp.c:1045:31: note: initialize the variable 'knotif' to silence this warning
> struct seccomp_knotif *knotif, *cur;
> ^
> = NULL
> 1 warning generated.
>
I'm curious as to how you got clang to generate this warning. I'm running with clang 10, and
upon running with V=1, and adding -Wsometimes-uninitialized, I'm not seeing this warning.
The following is the command called:
/usr/bin/clang-10 -Wp,-MD,kernel/.seccomp.o.d -nostdinc -isystem /usr/lib/llvm-10/lib/clang/10.0.0/include -I./arch/x86/include -I./arch/x86/include/generated -I./include -I./arch/x86/include/uapi -I./arch/x86/include/generated/uapi -I./include/uapi -I./include/generated/uapi -include ./include/linux/kconfig.h -include ./include/linux/compiler_types.h -D__KERNEL__ -Qunused-arguments -Wsometimes-uninitialized -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE -Werror=implicit-function-declaration -Werror=implicit-int -Wno-format-security -std=gnu89 -no-integrated-as -Werror=unknown-warning-option -mno-sse -mno-mmx -mno-sse2 -mno-3dnow -mno-avx -m64 -mno-80387 -mstack-alignment=8 -mtune=generic -mno-red-zone -mcmodel=kernel -DCONFIG_X86_X32_ABI -Wno-sign-compare -fno-asynchronous-unwind-tables -mretpoline-external-thunk -fno-delete-null-pointer-checks -Wno-address-of-packed-member -O2 -Wframe-larger-than=1024 -fstack-protector -Wno-format-invalid-specifier -Wno-gnu -mno-global-merge -Wno-unused-const-variable -g -pg -mfentry -DCC_USING_FENTRY -Wdeclaration-after-statement -Wvla -Wno-pointer-sign -Wno-array-bounds -fno-strict-overflow -fno-merge-all-constants -fno-stack-check -Werror=date-time -Werror=incompatible-pointer-types -fmacro-prefix-map=./= -fcf-protection=none -Wno-initializer-overrides -Wno-format -Wno-sign-compare -Wno-format-zero-length -Wno-tautological-constant-out-of-range-compare -DKBUILD_MODFILE='"kernel/seccomp"' -DKBUILD_BASENAME='"seccomp"' -DKBUILD_MODNAME='"seccomp"' -c -o kernel/seccomp.o kernel/seccomp.c
> > + struct seccomp_knotif *knotif;
> > u64 id;
> > long ret;
> >
> > @@ -1161,16 +1166,12 @@ static long seccomp_notify_id_valid(struct seccomp_filter *filter,
> > if (ret < 0)
> > return ret;
> >
> > - ret = -ENOENT;
> > - list_for_each_entry(knotif, &filter->notif->notifications, list) {
> > - if (knotif->id == id) {
> > - if (knotif->state == SECCOMP_NOTIFY_SENT)
> > - ret = 0;
> > - goto out;
> > - }
> > - }
> > + knotif = find_notification(filter, id);
> > + if (knotif && knotif->state == SECCOMP_NOTIFY_SENT)
> > + ret = 0;
> > + else
> > + ret = -ENOENT;
> >
> > -out:
> > mutex_unlock(&filter->notify_lock);
> > return ret;
> > }
> > --
> > 2.25.1
> >
>
> Cheers,
> Nathan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] seccomp: Add find_notification helper
2020-06-18 5:44 ` Sargun Dhillon
@ 2020-06-18 6:04 ` Nathan Chancellor
0 siblings, 0 replies; 6+ messages in thread
From: Nathan Chancellor @ 2020-06-18 6:04 UTC (permalink / raw)
To: Sargun Dhillon
Cc: containers, keescook, linux-api, linux-kernel, viro,
christian.brauner, cyphar, jannh, jeffv, palmer, rsesek, tycho,
Matt Denton, Kees Cook
On Thu, Jun 18, 2020 at 05:44:14AM +0000, Sargun Dhillon wrote:
> On Wed, Jun 17, 2020 at 01:08:44PM -0700, Nathan Chancellor wrote:
> > On Mon, Jun 01, 2020 at 04:25:32AM -0700, Sargun Dhillon wrote:
> > > This adds a helper which can iterate through a seccomp_filter to
> > > find a notification matching an ID. It removes several replicated
> > > chunks of code.
> > >
> > > Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> > > Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
> > > Reviewed-by: Tycho Andersen <tycho@tycho.ws>
> > > Cc: Matt Denton <mpdenton@google.com>
> > > Cc: Kees Cook <keescook@google.com>,
> > > Cc: Jann Horn <jannh@google.com>,
> > > Cc: Robert Sesek <rsesek@google.com>,
> > > Cc: Chris Palmer <palmer@google.com>
> > > Cc: Christian Brauner <christian.brauner@ubuntu.com>
> > > Cc: Tycho Andersen <tycho@tycho.ws>
> > > ---
> > > kernel/seccomp.c | 55 ++++++++++++++++++++++++------------------------
> > > 1 file changed, 28 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > > index 55a6184f5990..cc6b47173a95 100644
> > > --- a/kernel/seccomp.c
> > > +++ b/kernel/seccomp.c
> > > @@ -41,6 +41,7 @@
> > > #include <linux/tracehook.h>
> > > #include <linux/uaccess.h>
> > > #include <linux/anon_inodes.h>
> > > +#include <linux/lockdep.h>
> > >
> > > enum notify_state {
> > > SECCOMP_NOTIFY_INIT,
> > > @@ -1021,10 +1022,27 @@ static int seccomp_notify_release(struct inode *inode, struct file *file)
> > > return 0;
> > > }
> > >
> > > +/* must be called with notif_lock held */
> > > +static inline struct seccomp_knotif *
> > > +find_notification(struct seccomp_filter *filter, u64 id)
> > > +{
> > > + struct seccomp_knotif *cur;
> > > +
> > > + lockdep_assert_held(&filter->notify_lock);
> > > +
> > > + list_for_each_entry(cur, &filter->notif->notifications, list) {
> > > + if (cur->id == id)
> > > + return cur;
> > > + }
> > > +
> > > + return NULL;
> > > +}
> > > +
> > > +
> > > static long seccomp_notify_recv(struct seccomp_filter *filter,
> > > void __user *buf)
> > > {
> > > - struct seccomp_knotif *knotif = NULL, *cur;
> > > + struct seccomp_knotif *knotif, *cur;
> > > struct seccomp_notif unotif;
> > > ssize_t ret;
> > >
> > > @@ -1078,15 +1096,8 @@ static long seccomp_notify_recv(struct seccomp_filter *filter,
> > > * may have died when we released the lock, so we need to make
> > > * sure it's still around.
> > > */
> > > - knotif = NULL;
> > > mutex_lock(&filter->notify_lock);
> > > - list_for_each_entry(cur, &filter->notif->notifications, list) {
> > > - if (cur->id == unotif.id) {
> > > - knotif = cur;
> > > - break;
> > > - }
> > > - }
> > > -
> > > + knotif = find_notification(filter, unotif.id);
> > > if (knotif) {
> > > knotif->state = SECCOMP_NOTIFY_INIT;
> > > up(&filter->notif->request);
> > > @@ -1101,7 +1112,7 @@ static long seccomp_notify_send(struct seccomp_filter *filter,
> > > void __user *buf)
> > > {
> > > struct seccomp_notif_resp resp = {};
> > > - struct seccomp_knotif *knotif = NULL, *cur;
> > > + struct seccomp_knotif *knotif;
> > > long ret;
> > >
> > > if (copy_from_user(&resp, buf, sizeof(resp)))
> > > @@ -1118,13 +1129,7 @@ static long seccomp_notify_send(struct seccomp_filter *filter,
> > > if (ret < 0)
> > > return ret;
> > >
> > > - list_for_each_entry(cur, &filter->notif->notifications, list) {
> > > - if (cur->id == resp.id) {
> > > - knotif = cur;
> > > - break;
> > > - }
> > > - }
> > > -
> > > + knotif = find_notification(filter, resp.id);
> > > if (!knotif) {
> > > ret = -ENOENT;
> > > goto out;
> > > @@ -1150,7 +1155,7 @@ static long seccomp_notify_send(struct seccomp_filter *filter,
> > > static long seccomp_notify_id_valid(struct seccomp_filter *filter,
> > > void __user *buf)
> > > {
> > > - struct seccomp_knotif *knotif = NULL;
> >
> > I don't know that this should have been removed, clang now warns:
> >
> > kernel/seccomp.c:1063:2: warning: variable 'knotif' is used uninitialized whenever 'for' loop exits because its condition is false [-Wsometimes-uninitialized]
> > list_for_each_entry(cur, &filter->notif->notifications, list) {
> > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > include/linux/list.h:602:7: note: expanded from macro 'list_for_each_entry'
> > &pos->member != (head); \
> > ^~~~~~~~~~~~~~~~~~~~~~
> > kernel/seccomp.c:1075:7: note: uninitialized use occurs here
> > if (!knotif) {
> > ^~~~~~
> > kernel/seccomp.c:1063:2: note: remove the condition if it is always true
> > list_for_each_entry(cur, &filter->notif->notifications, list) {
> > ^
> > include/linux/list.h:602:7: note: expanded from macro 'list_for_each_entry'
> > &pos->member != (head); \
> > ^
> > kernel/seccomp.c:1045:31: note: initialize the variable 'knotif' to silence this warning
> > struct seccomp_knotif *knotif, *cur;
> > ^
> > = NULL
> > 1 warning generated.
> >
> I'm curious as to how you got clang to generate this warning. I'm running with clang 10, and
> upon running with V=1, and adding -Wsometimes-uninitialized, I'm not seeing this warning.
> The following is the command called:
> /usr/bin/clang-10 -Wp,-MD,kernel/.seccomp.o.d -nostdinc -isystem /usr/lib/llvm-10/lib/clang/10.0.0/include -I./arch/x86/include -I./arch/x86/include/generated -I./include -I./arch/x86/include/uapi -I./arch/x86/include/generated/uapi -I./include/uapi -I./include/generated/uapi -include ./include/linux/kconfig.h -include ./include/linux/compiler_types.h -D__KERNEL__ -Qunused-arguments -Wsometimes-uninitialized -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE -Werror=implicit-function-declaration -Werror=implicit-int -Wno-format-security -std=gnu89 -no-integrated-as -Werror=unknown-warning-option -mno-sse -mno-mmx -mno-sse2 -mno-3dnow -mno-avx -m64 -mno-80387 -mstack-alignment=8 -mtune=generic -mno-red-zone -mcmodel=kernel -DCONFIG_X86_X32_ABI -Wno-sign-compare -fno-asynchronous-unwind-tables -mretpoline-external-thunk -fno-delete-null-pointer-checks -Wno-address-of-packed-member -O2 -Wframe-larger-than=1024 -fstack-protector -Wno-format-invalid-specifier -Wno-gnu -mno-global-merge -Wno-unused-const-variable -g -pg -mfentry -DCC_USING_FENTRY -Wdeclaration-after-statement -Wvla -Wno-pointer-sign -Wno-array-bounds -fno-strict-overflow -fno-merge-all-constants -fno-stack-check -Werror=date-time -Werror=incompatible-pointer-types -fmacro-prefix-map=./= -fcf-protection=none -Wno-initializer-overrides -Wno-format -Wno-sign-compare -Wno-format-zero-length -Wno-tautological-constant-out-of-range-compare -DKBUILD_MODFILE='"kernel/seccomp"' -DKBUILD_BASENAME='"seccomp"' -DKBUILD_MODNAME='"seccomp"' -c -o kernel/seccomp.o kernel/seccomp.c
I saw it on a variety of configs but the one that is probably the
easiest to reproduce with is arm32 defconfig:
$ curl -LSs https://lore.kernel.org/lkml/20200601112532.150158-1-sargun@sargun.me/raw | git am
$ make -skj"$(nproc)" ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- LLVM=1 O=out/arm distclean defconfig kernel/seccomp.o
Cheers,
Nathan
> > > + struct seccomp_knotif *knotif;
> > > u64 id;
> > > long ret;
> > >
> > > @@ -1161,16 +1166,12 @@ static long seccomp_notify_id_valid(struct seccomp_filter *filter,
> > > if (ret < 0)
> > > return ret;
> > >
> > > - ret = -ENOENT;
> > > - list_for_each_entry(knotif, &filter->notif->notifications, list) {
> > > - if (knotif->id == id) {
> > > - if (knotif->state == SECCOMP_NOTIFY_SENT)
> > > - ret = 0;
> > > - goto out;
> > > - }
> > > - }
> > > + knotif = find_notification(filter, id);
> > > + if (knotif && knotif->state == SECCOMP_NOTIFY_SENT)
> > > + ret = 0;
> > > + else
> > > + ret = -ENOENT;
> > >
> > > -out:
> > > mutex_unlock(&filter->notify_lock);
> > > return ret;
> > > }
> > > --
> > > 2.25.1
> > >
> >
> > Cheers,
> > Nathan
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-06-18 6:04 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-01 11:25 [PATCH v3] seccomp: Add find_notification helper Sargun Dhillon
2020-06-01 18:26 ` Kees Cook
2020-06-17 20:08 ` Nathan Chancellor
2020-06-17 21:22 ` Kees Cook
2020-06-18 5:44 ` Sargun Dhillon
2020-06-18 6:04 ` Nathan Chancellor
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).