[v2,1/3] seccomp: Add find_notification helper

Submitted by Sargun Dhillon on May 28, 2020, 11:08 a.m.

Details

Message ID 20200528110858.3265-2-sargun@sargun.me
State New
Series "Add seccomp notifier ioctl that enables adding fds"
Headers show

Commit Message

Sargun Dhillon May 28, 2020, 11:08 a.m.
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>
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 | 51 ++++++++++++++++++++++++------------------------
 1 file changed, 25 insertions(+), 26 deletions(-)

Patch hide | download patch | download mbox

diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 55a6184f5990..94ae4c7502cc 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -1021,10 +1021,25 @@  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;
+
+	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,14 +1093,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;
@@ -1101,7 +1110,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 +1127,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 +1153,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 +1164,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;
 }

Comments

Kees Cook May 29, 2020, 6:23 a.m.
On Thu, May 28, 2020 at 04:08:56AM -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.

Nice, yes. I was noticing this redundancy too while I was looking at
notify locking earlier today. One note below...

> +/* must be called with notif_lock held */
> +static inline struct seccomp_knotif *
> +find_notification(struct seccomp_filter *filter, u64 id)
> +{
> +	struct seccomp_knotif *cur;

While the comment is good, let's actually enforce this with:

if (WARN_ON(!mutex_is_locked(&filter->notif_lock)))
	return NULL;

> +
> +	list_for_each_entry(cur, &filter->notif->notifications, list) {
> +		if (cur->id == id)
> +			return cur;
> +	}
> +
> +	return NULL;
> +}

Everything else looks good!
Christian Brauner May 29, 2020, 9:57 a.m.
On Thu, May 28, 2020 at 04:08:56AM -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>
> 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>
> ---

A single nit below otherwise:

Acked-by: Christian Brauner <christian.brauner@ubuntu.com>

>  kernel/seccomp.c | 51 ++++++++++++++++++++++++------------------------
>  1 file changed, 25 insertions(+), 26 deletions(-)
> 
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 55a6184f5990..94ae4c7502cc 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -1021,10 +1021,25 @@ 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;
> +
> +	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,14 +1093,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) {

Nit: additional \n which isn't present before any of the other new
find_notification() invocations.
Sargun Dhillon May 29, 2020, 5:40 p.m.
> 
> While the comment is good, let's actually enforce this with:
> 
> if (WARN_ON(!mutex_is_locked(&filter->notif_lock)))
> 	return NULL;
> 
I don't see much use of lockdep in seccomp (well, any), but
wouldn't a stronger statement be to use lockdep, and just have:

lockdep_assert_held(&filter->notify_lock);

As that checks that the lock is held by the current task.
Although, that does put this check behind lockdep, which means
that running in "normal" circumstances is less safe (but faster?).
Kees Cook May 29, 2020, 8:14 p.m.
On Fri, May 29, 2020 at 05:40:38PM +0000, Sargun Dhillon wrote:
> > 
> > While the comment is good, let's actually enforce this with:
> > 
> > if (WARN_ON(!mutex_is_locked(&filter->notif_lock)))
> > 	return NULL;
> > 
> I don't see much use of lockdep in seccomp (well, any), but
> wouldn't a stronger statement be to use lockdep, and just have:
> 
> lockdep_assert_held(&filter->notify_lock);
> 
> As that checks that the lock is held by the current task.

/me slaps his forehead

Yes. I need more coffee or something. Yes, I meant
lockdep_assert_held(), and now I need to go fix my pstore series since I
confused myself into the wrong function and using it so many times in
pstore overwrote the correct function in my head. Thank you!

> Although, that does put this check behind lockdep, which means
> that running in "normal" circumstances is less safe (but faster?).

Now, that's fine. The check needs to be "am *I* holding this mutex?" and
I don't think anything except lockdep can do that.