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 <sar...@sargun.me>
> Acked-by: Christian Brauner <christian.brau...@ubuntu.com>
> Reviewed-by: Tycho Andersen <ty...@tycho.ws>
> Cc: Matt Denton <mpden...@google.com>
> Cc: Kees Cook <keesc...@google.com>,
> Cc: Jann Horn <ja...@google.com>,
> Cc: Robert Sesek <rse...@google.com>,
> Cc: Chris Palmer <pal...@google.com>
> Cc: Christian Brauner <christian.brau...@ubuntu.com>
> Cc: Tycho Andersen <ty...@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
