Re: [PATCH v3] seccomp: Add find_notification helper
On Thu, Jun 18, 2020 at 05:44:14AM +, 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
> > > Acked-by: Christian Brauner
> > > Reviewed-by: Tycho Andersen
> > > Cc: Matt Denton
> > > Cc: Kees Cook ,
> > > Cc: Jann Horn ,
> > > Cc: Robert Sesek ,
> > > Cc: Chris Palmer
> > > Cc: Christian Brauner
> > > Cc: Tycho Andersen
> > > ---
> > > 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
> > > #include
> > > #include
> > > +#include
> > >
> > > 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(>notify_lock);
> > > +
> > > + list_for_each_entry(cur, >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(>notify_lock);
> > > - list_for_each_entry(cur, >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(>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(, 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, >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, >notif->notifications, list) {
> > ^
> > include/linux/list.h:602:7: note: expanded from macro 'list_for_each_entry'
> > >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, >notif->notifications, list) {
> > ^
> > include/linux/list.h:602:7: note: expanded from macro 'list_for_each_entry'
> > >member != (head);\
> > ^
> >
Re: [PATCH v3] seccomp: Add find_notification helper
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
> > Acked-by: Christian Brauner
> > Reviewed-by: Tycho Andersen
> > Cc: Matt Denton
> > Cc: Kees Cook ,
> > Cc: Jann Horn ,
> > Cc: Robert Sesek ,
> > Cc: Chris Palmer
> > Cc: Christian Brauner
> > Cc: Tycho Andersen
> > ---
> > 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
> > #include
> > #include
> > +#include
> >
> > 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(>notify_lock);
> > +
> > + list_for_each_entry(cur, >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(>notify_lock);
> > - list_for_each_entry(cur, >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(>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(, 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, >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, >notif->notifications, list) {
> ^
> include/linux/list.h:602:7: note: expanded from macro 'list_for_each_entry'
> >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, >notif->notifications, list) {
> ^
> include/linux/list.h:602:7: note: expanded from macro 'list_for_each_entry'
> >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.
>
Re: [PATCH v3] seccomp: Add find_notification helper
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, >notif->notifications, list) {
> ^
> include/linux/list.h:602:7: note: expanded from macro 'list_for_each_entry'
> >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, >notif->notifications, list) {
> ^
> include/linux/list.h:602:7: note: expanded from macro 'list_for_each_entry'
> >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
Re: [PATCH v3] seccomp: Add find_notification helper
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
> Acked-by: Christian Brauner
> Reviewed-by: Tycho Andersen
> Cc: Matt Denton
> Cc: Kees Cook ,
> Cc: Jann Horn ,
> Cc: Robert Sesek ,
> Cc: Chris Palmer
> Cc: Christian Brauner
> Cc: Tycho Andersen
> ---
> 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
> #include
> #include
> +#include
>
> 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(>notify_lock);
> +
> + list_for_each_entry(cur, >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(>notify_lock);
> - list_for_each_entry(cur, >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(>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(, 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, >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, >notif->notifications, list) {
^
include/linux/list.h:602:7: note: expanded from macro 'list_for_each_entry'
>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, >notif->notifications, list) {
^
include/linux/list.h:602:7: note: expanded from macro 'list_for_each_entry'
>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 =
Re: [PATCH v3] seccomp: Add find_notification helper
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 > Acked-by: Christian Brauner > Reviewed-by: Tycho Andersen > Cc: Matt Denton > Cc: Kees Cook , > Cc: Jann Horn , > Cc: Robert Sesek , > Cc: Chris Palmer > Cc: Christian Brauner > Cc: Tycho Andersen Thanks! Applied to for-next/seccomp. -- Kees Cook
[PATCH v3] seccomp: Add find_notification helper
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
Acked-by: Christian Brauner
Reviewed-by: Tycho Andersen
Cc: Matt Denton
Cc: Kees Cook ,
Cc: Jann Horn ,
Cc: Robert Sesek ,
Cc: Chris Palmer
Cc: Christian Brauner
Cc: Tycho Andersen
---
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
#include
#include
+#include
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(>notify_lock);
+
+ list_for_each_entry(cur, >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(>notify_lock);
- list_for_each_entry(cur, >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(>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(, 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, >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, >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(>notify_lock);
return ret;
}
--
2.25.1
