* [PATCH] seccomp: Check flags on seccomp_notif is unset @ 2019-12-25 21:45 Sargun Dhillon 2019-12-26 11:52 ` Christian Brauner 0 siblings, 1 reply; 13+ messages in thread From: Sargun Dhillon @ 2019-12-25 21:45 UTC (permalink / raw) To: linux-kernel, linux-api; +Cc: tycho, jannh, christian.brauner, keescook This patch is a small change in enforcement of the uapi for SECCOMP_IOCTL_NOTIF_RECV ioctl. Specificaly, the datastructure which is passed (seccomp_notif), has a flags member. Previously that could be set to a nonsense value, and we would ignore it. This ensures that no flags are set. Signed-off-by: Sargun Dhillon <sargun@sargun.me> Cc: Kees Cook <keescook@chromium.org> --- kernel/seccomp.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/kernel/seccomp.c b/kernel/seccomp.c index 12d2227e5786..455925557490 100644 --- a/kernel/seccomp.c +++ b/kernel/seccomp.c @@ -1026,6 +1026,13 @@ static long seccomp_notify_recv(struct seccomp_filter *filter, struct seccomp_notif unotif; ssize_t ret; + if (copy_from_user(&unotif, buf, sizeof(unotif))) + return -EFAULT; + + /* flags is reserved right now, make sure it's unset */ + if (unotif.flags) + return -EINVAL; + memset(&unotif, 0, sizeof(unotif)); ret = down_interruptible(&filter->notif->request); -- 2.20.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] seccomp: Check flags on seccomp_notif is unset 2019-12-25 21:45 [PATCH] seccomp: Check flags on seccomp_notif is unset Sargun Dhillon @ 2019-12-26 11:52 ` Christian Brauner 2019-12-26 14:32 ` Aleksa Sarai 0 siblings, 1 reply; 13+ messages in thread From: Christian Brauner @ 2019-12-26 11:52 UTC (permalink / raw) To: Sargun Dhillon; +Cc: linux-kernel, linux-api, tycho, jannh, keescook On Wed, Dec 25, 2019 at 09:45:33PM +0000, Sargun Dhillon wrote: > This patch is a small change in enforcement of the uapi for > SECCOMP_IOCTL_NOTIF_RECV ioctl. Specificaly, the datastructure which is > passed (seccomp_notif), has a flags member. Previously that could be > set to a nonsense value, and we would ignore it. This ensures that > no flags are set. > > Signed-off-by: Sargun Dhillon <sargun@sargun.me> > Cc: Kees Cook <keescook@chromium.org> I'm fine with this since we soon want to make use of the flag argument when we add a flag to get a pidfd from the seccomp notifier on receive. The major users I could identify already pass in seccomp_notif with all fields set to 0. If we really break users we can always revert; this seems very unlikely to me though. One more question below, otherwise: Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com> > --- > kernel/seccomp.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c > index 12d2227e5786..455925557490 100644 > --- a/kernel/seccomp.c > +++ b/kernel/seccomp.c > @@ -1026,6 +1026,13 @@ static long seccomp_notify_recv(struct seccomp_filter *filter, > struct seccomp_notif unotif; > ssize_t ret; > > + if (copy_from_user(&unotif, buf, sizeof(unotif))) > + return -EFAULT; > + > + /* flags is reserved right now, make sure it's unset */ > + if (unotif.flags) > + return -EINVAL; > + Might it make sense to use err = copy_struct_from_user(&unotif, sizeof(unotif), buf, sizeof(unotif)); if (err) return err; This way we check that the whole struct is 0 and report an error as soon as one of the members is non-zero. That's more drastic but it'd ensure that other fields can be used in the future for whatever purposes. It would also let us get rid of the memset() below. > memset(&unotif, 0, sizeof(unotif)); > > ret = down_interruptible(&filter->notif->request); > -- > 2.20.1 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] seccomp: Check flags on seccomp_notif is unset 2019-12-26 11:52 ` Christian Brauner @ 2019-12-26 14:32 ` Aleksa Sarai 2019-12-26 14:34 ` Christian Brauner 2019-12-26 15:37 ` Tycho Andersen 0 siblings, 2 replies; 13+ messages in thread From: Aleksa Sarai @ 2019-12-26 14:32 UTC (permalink / raw) To: Christian Brauner Cc: Sargun Dhillon, linux-kernel, linux-api, tycho, jannh, keescook [-- Attachment #1: Type: text/plain, Size: 2562 bytes --] On 2019-12-26, Christian Brauner <christian.brauner@ubuntu.com> wrote: > On Wed, Dec 25, 2019 at 09:45:33PM +0000, Sargun Dhillon wrote: > > This patch is a small change in enforcement of the uapi for > > SECCOMP_IOCTL_NOTIF_RECV ioctl. Specificaly, the datastructure which is > > passed (seccomp_notif), has a flags member. Previously that could be > > set to a nonsense value, and we would ignore it. This ensures that > > no flags are set. > > > > Signed-off-by: Sargun Dhillon <sargun@sargun.me> > > Cc: Kees Cook <keescook@chromium.org> > > I'm fine with this since we soon want to make use of the flag argument > when we add a flag to get a pidfd from the seccomp notifier on receive. > The major users I could identify already pass in seccomp_notif with all > fields set to 0. If we really break users we can always revert; this > seems very unlikely to me though. > > One more question below, otherwise: > > Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com> > > > --- > > kernel/seccomp.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c > > index 12d2227e5786..455925557490 100644 > > --- a/kernel/seccomp.c > > +++ b/kernel/seccomp.c > > @@ -1026,6 +1026,13 @@ static long seccomp_notify_recv(struct seccomp_filter *filter, > > struct seccomp_notif unotif; > > ssize_t ret; > > > > + if (copy_from_user(&unotif, buf, sizeof(unotif))) > > + return -EFAULT; > > + > > + /* flags is reserved right now, make sure it's unset */ > > + if (unotif.flags) > > + return -EINVAL; > > + > > Might it make sense to use > > err = copy_struct_from_user(&unotif, sizeof(unotif), buf, sizeof(unotif)); > if (err) > return err; > > This way we check that the whole struct is 0 and report an error as soon > as one of the members is non-zero. That's more drastic but it'd ensure > that other fields can be used in the future for whatever purposes. > It would also let us get rid of the memset() below. Given that this isn't an extensible struct, it would be simpler to just do check_zeroed_user() -- copy_struct_from_user() is overkill. That would also remove the need for any copy_from_user()s and the memset can be dropped by just doing struct seccomp_notif unotif = {}; > > memset(&unotif, 0, sizeof(unotif)); > > > > ret = down_interruptible(&filter->notif->request); > > -- > > 2.20.1 > > -- Aleksa Sarai Senior Software Engineer (Containers) SUSE Linux GmbH <https://www.cyphar.com/> [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] seccomp: Check flags on seccomp_notif is unset 2019-12-26 14:32 ` Aleksa Sarai @ 2019-12-26 14:34 ` Christian Brauner 2019-12-27 2:24 ` Aleksa Sarai 2019-12-26 15:37 ` Tycho Andersen 1 sibling, 1 reply; 13+ messages in thread From: Christian Brauner @ 2019-12-26 14:34 UTC (permalink / raw) To: Aleksa Sarai Cc: Sargun Dhillon, linux-kernel, linux-api, tycho, jannh, keescook On December 26, 2019 3:32:29 PM GMT+01:00, Aleksa Sarai <cyphar@cyphar.com> wrote: >On 2019-12-26, Christian Brauner <christian.brauner@ubuntu.com> wrote: >> On Wed, Dec 25, 2019 at 09:45:33PM +0000, Sargun Dhillon wrote: >> > This patch is a small change in enforcement of the uapi for >> > SECCOMP_IOCTL_NOTIF_RECV ioctl. Specificaly, the datastructure >which is >> > passed (seccomp_notif), has a flags member. Previously that could >be >> > set to a nonsense value, and we would ignore it. This ensures that >> > no flags are set. >> > >> > Signed-off-by: Sargun Dhillon <sargun@sargun.me> >> > Cc: Kees Cook <keescook@chromium.org> >> >> I'm fine with this since we soon want to make use of the flag >argument >> when we add a flag to get a pidfd from the seccomp notifier on >receive. >> The major users I could identify already pass in seccomp_notif with >all >> fields set to 0. If we really break users we can always revert; this >> seems very unlikely to me though. >> >> One more question below, otherwise: >> >> Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com> >> >> > --- >> > kernel/seccomp.c | 7 +++++++ >> > 1 file changed, 7 insertions(+) >> > >> > diff --git a/kernel/seccomp.c b/kernel/seccomp.c >> > index 12d2227e5786..455925557490 100644 >> > --- a/kernel/seccomp.c >> > +++ b/kernel/seccomp.c >> > @@ -1026,6 +1026,13 @@ static long seccomp_notify_recv(struct >seccomp_filter *filter, >> > struct seccomp_notif unotif; >> > ssize_t ret; >> > >> > + if (copy_from_user(&unotif, buf, sizeof(unotif))) >> > + return -EFAULT; >> > + >> > + /* flags is reserved right now, make sure it's unset */ >> > + if (unotif.flags) >> > + return -EINVAL; >> > + >> >> Might it make sense to use >> >> err = copy_struct_from_user(&unotif, sizeof(unotif), buf, >sizeof(unotif)); >> if (err) >> return err; >> >> This way we check that the whole struct is 0 and report an error as >soon >> as one of the members is non-zero. That's more drastic but it'd >ensure >> that other fields can be used in the future for whatever purposes. >> It would also let us get rid of the memset() below. > >Given that this isn't an extensible struct, it would be simpler to just >do >check_zeroed_user() -- copy_struct_from_user() is overkill. That would >also remove the need for any copy_from_user()s and the memset can be >dropped by just doing > > struct seccomp_notif unotif = {}; > >> > memset(&unotif, 0, sizeof(unotif)); >> > >> > ret = down_interruptible(&filter->notif->request); >> > -- >> > 2.20.1 >> > It is an extensible struct. That's why we have notifier size checking built in. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] seccomp: Check flags on seccomp_notif is unset 2019-12-26 14:34 ` Christian Brauner @ 2019-12-27 2:24 ` Aleksa Sarai 2019-12-27 2:31 ` Aleksa Sarai 0 siblings, 1 reply; 13+ messages in thread From: Aleksa Sarai @ 2019-12-27 2:24 UTC (permalink / raw) To: Christian Brauner Cc: Sargun Dhillon, linux-kernel, linux-api, tycho, jannh, keescook [-- Attachment #1: Type: text/plain, Size: 3564 bytes --] On 2019-12-26, Christian Brauner <christian.brauner@ubuntu.com> wrote: > On December 26, 2019 3:32:29 PM GMT+01:00, Aleksa Sarai <cyphar@cyphar.com> wrote: > >On 2019-12-26, Christian Brauner <christian.brauner@ubuntu.com> wrote: > >> On Wed, Dec 25, 2019 at 09:45:33PM +0000, Sargun Dhillon wrote: > >> > This patch is a small change in enforcement of the uapi for > >> > SECCOMP_IOCTL_NOTIF_RECV ioctl. Specificaly, the datastructure > >which is > >> > passed (seccomp_notif), has a flags member. Previously that could > >be > >> > set to a nonsense value, and we would ignore it. This ensures that > >> > no flags are set. > >> > > >> > Signed-off-by: Sargun Dhillon <sargun@sargun.me> > >> > Cc: Kees Cook <keescook@chromium.org> > >> > >> I'm fine with this since we soon want to make use of the flag > >argument > >> when we add a flag to get a pidfd from the seccomp notifier on > >receive. > >> The major users I could identify already pass in seccomp_notif with > >all > >> fields set to 0. If we really break users we can always revert; this > >> seems very unlikely to me though. > >> > >> One more question below, otherwise: > >> > >> Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com> > >> > >> > --- > >> > kernel/seccomp.c | 7 +++++++ > >> > 1 file changed, 7 insertions(+) > >> > > >> > diff --git a/kernel/seccomp.c b/kernel/seccomp.c > >> > index 12d2227e5786..455925557490 100644 > >> > --- a/kernel/seccomp.c > >> > +++ b/kernel/seccomp.c > >> > @@ -1026,6 +1026,13 @@ static long seccomp_notify_recv(struct > >seccomp_filter *filter, > >> > struct seccomp_notif unotif; > >> > ssize_t ret; > >> > > >> > + if (copy_from_user(&unotif, buf, sizeof(unotif))) > >> > + return -EFAULT; > >> > + > >> > + /* flags is reserved right now, make sure it's unset */ > >> > + if (unotif.flags) > >> > + return -EINVAL; > >> > + > >> > >> Might it make sense to use > >> > >> err = copy_struct_from_user(&unotif, sizeof(unotif), buf, > >sizeof(unotif)); > >> if (err) > >> return err; > >> > >> This way we check that the whole struct is 0 and report an error as > >soon > >> as one of the members is non-zero. That's more drastic but it'd > >ensure > >> that other fields can be used in the future for whatever purposes. > >> It would also let us get rid of the memset() below. > > > >Given that this isn't an extensible struct, it would be simpler to just > >do > >check_zeroed_user() -- copy_struct_from_user() is overkill. That would > >also remove the need for any copy_from_user()s and the memset can be > >dropped by just doing > > > > struct seccomp_notif unotif = {}; > > > >> > memset(&unotif, 0, sizeof(unotif)); > >> > > >> > ret = down_interruptible(&filter->notif->request); > >> > -- > >> > 2.20.1 > >> > > > It is an extensible struct. That's why we have notifier size checking built in. Ah right, NOTIF_GET_SIZES. I reckon check_zeroed_user() is still a bit simpler since none of the fields are used right now (and really, this patch should be checking all of them, not just ->flags, if we want to use any of them in the future). But sure, copy_struct_from_user() also makes sense since it is extensible (though I personally do find the whole NOTIF_GET_SIZES thing a bit scary -- but that's water under the bridge at this point, and as long as userspace is clever enough it shouldn't be a problem). -- Aleksa Sarai Senior Software Engineer (Containers) SUSE Linux GmbH <https://www.cyphar.com/> [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] seccomp: Check flags on seccomp_notif is unset 2019-12-27 2:24 ` Aleksa Sarai @ 2019-12-27 2:31 ` Aleksa Sarai 2019-12-27 11:47 ` Christian Brauner 0 siblings, 1 reply; 13+ messages in thread From: Aleksa Sarai @ 2019-12-27 2:31 UTC (permalink / raw) To: Christian Brauner Cc: Sargun Dhillon, linux-kernel, linux-api, tycho, jannh, keescook [-- Attachment #1: Type: text/plain, Size: 4071 bytes --] On 2019-12-27, Aleksa Sarai <cyphar@cyphar.com> wrote: > On 2019-12-26, Christian Brauner <christian.brauner@ubuntu.com> wrote: > > On December 26, 2019 3:32:29 PM GMT+01:00, Aleksa Sarai <cyphar@cyphar.com> wrote: > > >On 2019-12-26, Christian Brauner <christian.brauner@ubuntu.com> wrote: > > >> On Wed, Dec 25, 2019 at 09:45:33PM +0000, Sargun Dhillon wrote: > > >> > This patch is a small change in enforcement of the uapi for > > >> > SECCOMP_IOCTL_NOTIF_RECV ioctl. Specificaly, the datastructure > > >which is > > >> > passed (seccomp_notif), has a flags member. Previously that could > > >be > > >> > set to a nonsense value, and we would ignore it. This ensures that > > >> > no flags are set. > > >> > > > >> > Signed-off-by: Sargun Dhillon <sargun@sargun.me> > > >> > Cc: Kees Cook <keescook@chromium.org> > > >> > > >> I'm fine with this since we soon want to make use of the flag > > >argument > > >> when we add a flag to get a pidfd from the seccomp notifier on > > >receive. > > >> The major users I could identify already pass in seccomp_notif with > > >all > > >> fields set to 0. If we really break users we can always revert; this > > >> seems very unlikely to me though. > > >> > > >> One more question below, otherwise: > > >> > > >> Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com> > > >> > > >> > --- > > >> > kernel/seccomp.c | 7 +++++++ > > >> > 1 file changed, 7 insertions(+) > > >> > > > >> > diff --git a/kernel/seccomp.c b/kernel/seccomp.c > > >> > index 12d2227e5786..455925557490 100644 > > >> > --- a/kernel/seccomp.c > > >> > +++ b/kernel/seccomp.c > > >> > @@ -1026,6 +1026,13 @@ static long seccomp_notify_recv(struct > > >seccomp_filter *filter, > > >> > struct seccomp_notif unotif; > > >> > ssize_t ret; > > >> > > > >> > + if (copy_from_user(&unotif, buf, sizeof(unotif))) > > >> > + return -EFAULT; > > >> > + > > >> > + /* flags is reserved right now, make sure it's unset */ > > >> > + if (unotif.flags) > > >> > + return -EINVAL; > > >> > + > > >> > > >> Might it make sense to use > > >> > > >> err = copy_struct_from_user(&unotif, sizeof(unotif), buf, > > >sizeof(unotif)); > > >> if (err) > > >> return err; > > >> > > >> This way we check that the whole struct is 0 and report an error as > > >soon > > >> as one of the members is non-zero. That's more drastic but it'd > > >ensure > > >> that other fields can be used in the future for whatever purposes. > > >> It would also let us get rid of the memset() below. > > > > > >Given that this isn't an extensible struct, it would be simpler to just > > >do > > >check_zeroed_user() -- copy_struct_from_user() is overkill. That would > > >also remove the need for any copy_from_user()s and the memset can be > > >dropped by just doing > > > > > > struct seccomp_notif unotif = {}; > > > > > >> > memset(&unotif, 0, sizeof(unotif)); > > >> > > > >> > ret = down_interruptible(&filter->notif->request); > > >> > -- > > >> > 2.20.1 > > >> > > > > > It is an extensible struct. That's why we have notifier size checking built in. > > Ah right, NOTIF_GET_SIZES. I reckon check_zeroed_user() is still a bit > simpler since none of the fields are used right now (and really, this > patch should be checking all of them, not just ->flags, if we want to > use any of them in the future). Scratch that -- as Tycho just mentioned, there is un-named padding in the struct so check_zeroed_user() is the wrong thing to do. But this also will make extensions harder to deal with because (presumably) they will also have un-named padding, making copy_struct_from_user() the wrong thing to do as well. So while there's not much to be done to fix the current struct layout, I humbly suggest that any future struct extensions should not have any un-named padding (so that at the very least you could use copy_struct_from_user() in some form). -- Aleksa Sarai Senior Software Engineer (Containers) SUSE Linux GmbH <https://www.cyphar.com/> [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] seccomp: Check flags on seccomp_notif is unset 2019-12-27 2:31 ` Aleksa Sarai @ 2019-12-27 11:47 ` Christian Brauner 2019-12-27 14:22 ` Sargun Dhillon 0 siblings, 1 reply; 13+ messages in thread From: Christian Brauner @ 2019-12-27 11:47 UTC (permalink / raw) To: Aleksa Sarai Cc: Sargun Dhillon, linux-kernel, linux-api, tycho, jannh, keescook On Fri, Dec 27, 2019 at 01:31:31PM +1100, Aleksa Sarai wrote: > On 2019-12-27, Aleksa Sarai <cyphar@cyphar.com> wrote: > > On 2019-12-26, Christian Brauner <christian.brauner@ubuntu.com> wrote: > > > On December 26, 2019 3:32:29 PM GMT+01:00, Aleksa Sarai <cyphar@cyphar.com> wrote: > > > >On 2019-12-26, Christian Brauner <christian.brauner@ubuntu.com> wrote: > > > >> On Wed, Dec 25, 2019 at 09:45:33PM +0000, Sargun Dhillon wrote: > > > >> > This patch is a small change in enforcement of the uapi for > > > >> > SECCOMP_IOCTL_NOTIF_RECV ioctl. Specificaly, the datastructure > > > >which is > > > >> > passed (seccomp_notif), has a flags member. Previously that could > > > >be > > > >> > set to a nonsense value, and we would ignore it. This ensures that > > > >> > no flags are set. > > > >> > > > > >> > Signed-off-by: Sargun Dhillon <sargun@sargun.me> > > > >> > Cc: Kees Cook <keescook@chromium.org> > > > >> > > > >> I'm fine with this since we soon want to make use of the flag > > > >argument > > > >> when we add a flag to get a pidfd from the seccomp notifier on > > > >receive. > > > >> The major users I could identify already pass in seccomp_notif with > > > >all > > > >> fields set to 0. If we really break users we can always revert; this > > > >> seems very unlikely to me though. > > > >> > > > >> One more question below, otherwise: > > > >> > > > >> Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com> > > > >> > > > >> > --- > > > >> > kernel/seccomp.c | 7 +++++++ > > > >> > 1 file changed, 7 insertions(+) > > > >> > > > > >> > diff --git a/kernel/seccomp.c b/kernel/seccomp.c > > > >> > index 12d2227e5786..455925557490 100644 > > > >> > --- a/kernel/seccomp.c > > > >> > +++ b/kernel/seccomp.c > > > >> > @@ -1026,6 +1026,13 @@ static long seccomp_notify_recv(struct > > > >seccomp_filter *filter, > > > >> > struct seccomp_notif unotif; > > > >> > ssize_t ret; > > > >> > > > > >> > + if (copy_from_user(&unotif, buf, sizeof(unotif))) > > > >> > + return -EFAULT; > > > >> > + > > > >> > + /* flags is reserved right now, make sure it's unset */ > > > >> > + if (unotif.flags) > > > >> > + return -EINVAL; > > > >> > + > > > >> > > > >> Might it make sense to use > > > >> > > > >> err = copy_struct_from_user(&unotif, sizeof(unotif), buf, > > > >sizeof(unotif)); > > > >> if (err) > > > >> return err; > > > >> > > > >> This way we check that the whole struct is 0 and report an error as > > > >soon > > > >> as one of the members is non-zero. That's more drastic but it'd > > > >ensure > > > >> that other fields can be used in the future for whatever purposes. > > > >> It would also let us get rid of the memset() below. > > > > > > > >Given that this isn't an extensible struct, it would be simpler to just > > > >do > > > >check_zeroed_user() -- copy_struct_from_user() is overkill. That would > > > >also remove the need for any copy_from_user()s and the memset can be > > > >dropped by just doing > > > > > > > > struct seccomp_notif unotif = {}; > > > > > > > >> > memset(&unotif, 0, sizeof(unotif)); > > > >> > > > > >> > ret = down_interruptible(&filter->notif->request); > > > >> > -- > > > >> > 2.20.1 > > > >> > > > > > > > It is an extensible struct. That's why we have notifier size checking built in. > > > > Ah right, NOTIF_GET_SIZES. I reckon check_zeroed_user() is still a bit > > simpler since none of the fields are used right now (and really, this > > patch should be checking all of them, not just ->flags, if we want to > > use any of them in the future). > > Scratch that -- as Tycho just mentioned, there is un-named padding in > the struct so check_zeroed_user() is the wrong thing to do. But this Hm, I don't think so. I understood Tycho's point as _if_ there ever is padding then this would not be zeroed. Right now, there is no padding since the struct is correctly padded: struct seccomp_data { int nr; __u32 arch; __u64 instruction_pointer; __u64 args[6]; }; struct seccomp_notif { __u64 id; __u32 pid; __u32 flags; struct seccomp_data data; }; which would be - using pahole: struct seccomp_data { int nr; /* 0 4 */ __u32 arch; /* 4 4 */ __u64 instruction_pointer; /* 8 8 */ __u64 args[6]; /* 16 48 */ /* size: 64, cachelines: 1, members: 4 */ }; struct seccomp_notif { __u64 id; /* 0 8 */ __u32 pid; /* 8 4 */ __u32 flags; /* 12 4 */ struct seccomp_data data; /* 16 64 */ /* size: 80, cachelines: 2, members: 4 */ /* last cacheline: 16 bytes */ }; The only worry would be a 2byte int type but there's no architecture we support which does this right now afaict. > also will make extensions harder to deal with because (presumably) they > will also have un-named padding, making copy_struct_from_user() the This all will be a non-issue if we just use __u64 for extensions. My point about using copy_struct_from_user() was that we should verify that _all_ fields are uninitialized and not just the flags argument since we might introduce a flags argument that requires another already existing member in seccomp_notif to be set to a value. We should do this change now so we don't have to risk breaking someone in the future. I'm trying to get at least Mozilla/Firefox off of their crazy SECCOMP_RET_TRAP way of implementing their broker onto the user notifier and they will likely need some extensions. That includes the pidfd stuff for seccomp that Sargun will likely be doing and the new pidfd_getfd() syscall. So it's not unlikely that we might need other already existing fields in that struct to be set to some value. I don't particulary care how we do it: - We can do a simple copy_from_user() and check each field individually. - Use copy_struct_from_user(). That is safe to do right now since there is no padding afaict and it'll automatically verify new fields as well. If I understand the worry correctly then the argument against copy_struct_from_user() here is that there might be padding introduced and userspace will not do an explicit memset() but rather rely on an empty inializer {} and will _accidently_ pass down a struct which has __all fields cleared__ but __uninitialized padding__ and we tell them EINVAL? That can only happen if we introduce padding in the struct which I'd argue we just don't do. That'll be in line with what we require from our ABIs already anyway. Christian ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] seccomp: Check flags on seccomp_notif is unset 2019-12-27 11:47 ` Christian Brauner @ 2019-12-27 14:22 ` Sargun Dhillon 2019-12-27 14:38 ` Tycho Andersen 2019-12-27 15:15 ` Aleksa Sarai 0 siblings, 2 replies; 13+ messages in thread From: Sargun Dhillon @ 2019-12-27 14:22 UTC (permalink / raw) To: Christian Brauner Cc: Aleksa Sarai, LKML, Linux API, Tycho Andersen, Jann Horn, Kees Cook On Fri, Dec 27, 2019 at 6:47 AM Christian Brauner <christian.brauner@ubuntu.com> wrote: > > On Fri, Dec 27, 2019 at 01:31:31PM +1100, Aleksa Sarai wrote: > > On 2019-12-27, Aleksa Sarai <cyphar@cyphar.com> wrote: > > > > Scratch that -- as Tycho just mentioned, there is un-named padding in > > the struct so check_zeroed_user() is the wrong thing to do. But this > > Hm, I don't think so. > I understood Tycho's point as _if_ there ever is padding then this would > not be zeroed. > Right now, there is no padding since the struct is correctly padded: > > struct seccomp_data { > int nr; > __u32 arch; > __u64 instruction_pointer; > __u64 args[6]; > }; > > struct seccomp_notif { > __u64 id; > __u32 pid; > __u32 flags; > struct seccomp_data data; > }; > > which would be - using pahole: > > struct seccomp_data { > int nr; /* 0 4 */ > __u32 arch; /* 4 4 */ > __u64 instruction_pointer; /* 8 8 */ > __u64 args[6]; /* 16 48 */ > > /* size: 64, cachelines: 1, members: 4 */ > }; > struct seccomp_notif { > __u64 id; /* 0 8 */ > __u32 pid; /* 8 4 */ > __u32 flags; /* 12 4 */ > struct seccomp_data data; /* 16 64 */ > > /* size: 80, cachelines: 2, members: 4 */ > /* last cacheline: 16 bytes */ > }; > > The only worry would be a 2byte int type but there's no architecture > we support which does this right now afaict. > > > also will make extensions harder to deal with because (presumably) they > > will also have un-named padding, making copy_struct_from_user() the > > This all will be a non-issue if we just use __u64 for extensions. > > My point about using copy_struct_from_user() was that we should verify > that _all_ fields are uninitialized and not just the flags argument > since we might introduce a flags argument that requires another already > existing member in seccomp_notif to be set to a value. We should do this > change now so we don't have to risk breaking someone in the future. > > I'm trying to get at least Mozilla/Firefox off of their crazy > SECCOMP_RET_TRAP way of implementing their broker onto the user notifier > and they will likely need some extensions. That includes the pidfd stuff > for seccomp that Sargun will likely be doing and the new pidfd_getfd() > syscall. So it's not unlikely that we might need other already existing > fields in that struct to be set to some value. > > I don't particulary care how we do it: > - We can do a simple copy_from_user() and check each field individually. Just doing a simple copy_from_user, and for now, calling memchr_inv on the whole thing. We can drop the memset, and just leave a note to indicate that if unpadded fields are introduced in the future, this structure must be manually zeroed out. Although, this might be laying a trap for ourselves. This leaves us in a good position for introducing a flag field in the future. All we have to do is change the memchr_inv from checking on an entire struct basis to checking on a per-field basis. > - Use copy_struct_from_user(). > That is safe to do right now since there is no padding afaict and > it'll automatically verify new fields as well. > If I understand the worry correctly then the argument against > copy_struct_from_user() here is that there might be padding introduced > and userspace will not do an explicit memset() but rather rely on an > empty inializer {} and will _accidently_ pass down a struct which has > __all fields cleared__ but __uninitialized padding__ and we tell them > EINVAL? That can only happen if we introduce padding in the struct > which I'd argue we just don't do. That'll be in line with what we > require from our ABIs already anyway. > > Christian ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] seccomp: Check flags on seccomp_notif is unset 2019-12-27 14:22 ` Sargun Dhillon @ 2019-12-27 14:38 ` Tycho Andersen 2019-12-27 15:15 ` Aleksa Sarai 1 sibling, 0 replies; 13+ messages in thread From: Tycho Andersen @ 2019-12-27 14:38 UTC (permalink / raw) To: Sargun Dhillon Cc: Christian Brauner, Aleksa Sarai, LKML, Linux API, Jann Horn, Kees Cook On Fri, Dec 27, 2019 at 09:22:20AM -0500, Sargun Dhillon wrote: > Just doing a simple copy_from_user, and for now, calling memchr_inv > on the whole thing. We can drop the memset, and just leave a note to > indicate that if unpadded fields are introduced in the future, this structure > must be manually zeroed out. Although, this might be laying a trap for > ourselves. Yes, please keep the memset(). Tycho ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] seccomp: Check flags on seccomp_notif is unset 2019-12-27 14:22 ` Sargun Dhillon 2019-12-27 14:38 ` Tycho Andersen @ 2019-12-27 15:15 ` Aleksa Sarai 2019-12-27 15:32 ` Christian Brauner 1 sibling, 1 reply; 13+ messages in thread From: Aleksa Sarai @ 2019-12-27 15:15 UTC (permalink / raw) To: Sargun Dhillon Cc: Christian Brauner, LKML, Linux API, Tycho Andersen, Jann Horn, Kees Cook [-- Attachment #1: Type: text/plain, Size: 4713 bytes --] On 2019-12-27, Sargun Dhillon <sargun@sargun.me> wrote: > On Fri, Dec 27, 2019 at 6:47 AM Christian Brauner > <christian.brauner@ubuntu.com> wrote: > > > > On Fri, Dec 27, 2019 at 01:31:31PM +1100, Aleksa Sarai wrote: > > > On 2019-12-27, Aleksa Sarai <cyphar@cyphar.com> wrote: > > > > > > Scratch that -- as Tycho just mentioned, there is un-named padding in > > > the struct so check_zeroed_user() is the wrong thing to do. But this > > > > Hm, I don't think so. > > I understood Tycho's point as _if_ there ever is padding then this would > > not be zeroed. > > Right now, there is no padding since the struct is correctly padded: > > > > struct seccomp_data { > > int nr; > > __u32 arch; > > __u64 instruction_pointer; > > __u64 args[6]; > > }; > > > > struct seccomp_notif { > > __u64 id; > > __u32 pid; > > __u32 flags; > > struct seccomp_data data; > > }; > > > > which would be - using pahole: > > > > struct seccomp_data { > > int nr; /* 0 4 */ > > __u32 arch; /* 4 4 */ > > __u64 instruction_pointer; /* 8 8 */ > > __u64 args[6]; /* 16 48 */ > > > > /* size: 64, cachelines: 1, members: 4 */ > > }; > > struct seccomp_notif { > > __u64 id; /* 0 8 */ > > __u32 pid; /* 8 4 */ > > __u32 flags; /* 12 4 */ > > struct seccomp_data data; /* 16 64 */ > > > > /* size: 80, cachelines: 2, members: 4 */ > > /* last cacheline: 16 bytes */ > > }; > > > > The only worry would be a 2byte int type but there's no architecture > > we support which does this right now afaict. > > > > > also will make extensions harder to deal with because (presumably) they > > > will also have un-named padding, making copy_struct_from_user() the > > > > This all will be a non-issue if we just use __u64 for extensions. > > > > My point about using copy_struct_from_user() was that we should verify > > that _all_ fields are uninitialized and not just the flags argument > > since we might introduce a flags argument that requires another already > > existing member in seccomp_notif to be set to a value. We should do this > > change now so we don't have to risk breaking someone in the future. > > > > I'm trying to get at least Mozilla/Firefox off of their crazy > > SECCOMP_RET_TRAP way of implementing their broker onto the user notifier > > and they will likely need some extensions. That includes the pidfd stuff > > for seccomp that Sargun will likely be doing and the new pidfd_getfd() > > syscall. So it's not unlikely that we might need other already existing > > fields in that struct to be set to some value. > > > > I don't particulary care how we do it: > > - We can do a simple copy_from_user() and check each field individually. > > Just doing a simple copy_from_user, and for now, calling memchr_inv > on the whole thing. We can drop the memset, and just leave a note to > indicate that if unpadded fields are introduced in the future, this structure > must be manually zeroed out. Although, this might be laying a trap for > ourselves. > > This leaves us in a good position for introducing a flag field in the future. > All we have to do is change the memchr_inv from checking on an > entire struct basis to checking on a per-field basis. There is no need to do memchr_inv() on copy_from_user() to check for zero-ness. That's the entire point of check_zeroed_user() -- to not need to do it that way. > > - Use copy_struct_from_user(). > > That is safe to do right now since there is no padding afaict and > > it'll automatically verify new fields as well. > > If I understand the worry correctly then the argument against > > copy_struct_from_user() here is that there might be padding introduced > > and userspace will not do an explicit memset() but rather rely on an > > empty inializer {} and will _accidently_ pass down a struct which has > > __all fields cleared__ but __uninitialized padding__ and we tell them > > EINVAL? That can only happen if we introduce padding in the struct > > which I'd argue we just don't do. That'll be in line with what we > > require from our ABIs already anyway. -- Aleksa Sarai Senior Software Engineer (Containers) SUSE Linux GmbH <https://www.cyphar.com/> [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] seccomp: Check flags on seccomp_notif is unset 2019-12-27 15:15 ` Aleksa Sarai @ 2019-12-27 15:32 ` Christian Brauner 0 siblings, 0 replies; 13+ messages in thread From: Christian Brauner @ 2019-12-27 15:32 UTC (permalink / raw) To: Aleksa Sarai, Sargun Dhillon Cc: LKML, Linux API, Tycho Andersen, Jann Horn, Kees Cook On December 27, 2019 4:15:01 PM GMT+01:00, Aleksa Sarai <cyphar@cyphar.com> wrote: >On 2019-12-27, Sargun Dhillon <sargun@sargun.me> wrote: >> On Fri, Dec 27, 2019 at 6:47 AM Christian Brauner >> <christian.brauner@ubuntu.com> wrote: >> > >> > On Fri, Dec 27, 2019 at 01:31:31PM +1100, Aleksa Sarai wrote: >> > > On 2019-12-27, Aleksa Sarai <cyphar@cyphar.com> wrote: >> > > >> > > Scratch that -- as Tycho just mentioned, there is un-named >padding in >> > > the struct so check_zeroed_user() is the wrong thing to do. But >this >> > >> > Hm, I don't think so. >> > I understood Tycho's point as _if_ there ever is padding then this >would >> > not be zeroed. >> > Right now, there is no padding since the struct is correctly >padded: >> > >> > struct seccomp_data { >> > int nr; >> > __u32 arch; >> > __u64 instruction_pointer; >> > __u64 args[6]; >> > }; >> > >> > struct seccomp_notif { >> > __u64 id; >> > __u32 pid; >> > __u32 flags; >> > struct seccomp_data data; >> > }; >> > >> > which would be - using pahole: >> > >> > struct seccomp_data { >> > int nr; /* 0 > 4 */ >> > __u32 arch; /* 4 > 4 */ >> > __u64 instruction_pointer; /* 8 > 8 */ >> > __u64 args[6]; /* 16 > 48 */ >> > >> > /* size: 64, cachelines: 1, members: 4 */ >> > }; >> > struct seccomp_notif { >> > __u64 id; /* 0 > 8 */ >> > __u32 pid; /* 8 > 4 */ >> > __u32 flags; /* 12 > 4 */ >> > struct seccomp_data data; /* 16 > 64 */ >> > >> > /* size: 80, cachelines: 2, members: 4 */ >> > /* last cacheline: 16 bytes */ >> > }; >> > >> > The only worry would be a 2byte int type but there's no >architecture >> > we support which does this right now afaict. >> > >> > > also will make extensions harder to deal with because >(presumably) they >> > > will also have un-named padding, making copy_struct_from_user() >the >> > >> > This all will be a non-issue if we just use __u64 for extensions. >> > >> > My point about using copy_struct_from_user() was that we should >verify >> > that _all_ fields are uninitialized and not just the flags argument >> > since we might introduce a flags argument that requires another >already >> > existing member in seccomp_notif to be set to a value. We should do >this >> > change now so we don't have to risk breaking someone in the future. >> > >> > I'm trying to get at least Mozilla/Firefox off of their crazy >> > SECCOMP_RET_TRAP way of implementing their broker onto the user >notifier >> > and they will likely need some extensions. That includes the pidfd >stuff >> > for seccomp that Sargun will likely be doing and the new >pidfd_getfd() >> > syscall. So it's not unlikely that we might need other already >existing >> > fields in that struct to be set to some value. >> > >> > I don't particulary care how we do it: >> > - We can do a simple copy_from_user() and check each field >individually. >> >> Just doing a simple copy_from_user, and for now, calling memchr_inv >> on the whole thing. We can drop the memset, and just leave a note to >> indicate that if unpadded fields are introduced in the future, this >structure >> must be manually zeroed out. Although, this might be laying a trap >for >> ourselves. >> >> This leaves us in a good position for introducing a flag field in the >future. >> All we have to do is change the memchr_inv from checking on an >> entire struct basis to checking on a per-field basis. > >There is no need to do memchr_inv() on copy_from_user() to check for >zero-ness. That's the entire point of check_zeroed_user() -- to not >need >to do it that way. Right, we added that too a while ago. Let's use it. Christian ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] seccomp: Check flags on seccomp_notif is unset 2019-12-26 14:32 ` Aleksa Sarai 2019-12-26 14:34 ` Christian Brauner @ 2019-12-26 15:37 ` Tycho Andersen 2019-12-27 2:28 ` Aleksa Sarai 1 sibling, 1 reply; 13+ messages in thread From: Tycho Andersen @ 2019-12-26 15:37 UTC (permalink / raw) To: Aleksa Sarai Cc: Christian Brauner, Sargun Dhillon, linux-kernel, linux-api, jannh, keescook On Fri, Dec 27, 2019 at 01:32:29AM +1100, Aleksa Sarai wrote: > On 2019-12-26, Christian Brauner <christian.brauner@ubuntu.com> wrote: > > On Wed, Dec 25, 2019 at 09:45:33PM +0000, Sargun Dhillon wrote: > > > This patch is a small change in enforcement of the uapi for > > > SECCOMP_IOCTL_NOTIF_RECV ioctl. Specificaly, the datastructure which is > > > passed (seccomp_notif), has a flags member. Previously that could be > > > set to a nonsense value, and we would ignore it. This ensures that > > > no flags are set. > > > > > > Signed-off-by: Sargun Dhillon <sargun@sargun.me> > > > Cc: Kees Cook <keescook@chromium.org> > > > > I'm fine with this since we soon want to make use of the flag argument > > when we add a flag to get a pidfd from the seccomp notifier on receive. > > The major users I could identify already pass in seccomp_notif with all > > fields set to 0. If we really break users we can always revert; this > > seems very unlikely to me though. > > > > One more question below, otherwise: > > > > Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com> > > > > > --- > > > kernel/seccomp.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c > > > index 12d2227e5786..455925557490 100644 > > > --- a/kernel/seccomp.c > > > +++ b/kernel/seccomp.c > > > @@ -1026,6 +1026,13 @@ static long seccomp_notify_recv(struct seccomp_filter *filter, > > > struct seccomp_notif unotif; > > > ssize_t ret; > > > > > > + if (copy_from_user(&unotif, buf, sizeof(unotif))) > > > + return -EFAULT; > > > + > > > + /* flags is reserved right now, make sure it's unset */ > > > + if (unotif.flags) > > > + return -EINVAL; > > > + > > > > Might it make sense to use > > > > err = copy_struct_from_user(&unotif, sizeof(unotif), buf, sizeof(unotif)); > > if (err) > > return err; > > > > This way we check that the whole struct is 0 and report an error as soon > > as one of the members is non-zero. That's more drastic but it'd ensure > > that other fields can be used in the future for whatever purposes. > > It would also let us get rid of the memset() below. > > Given that this isn't an extensible struct, it would be simpler to just do > check_zeroed_user() -- copy_struct_from_user() is overkill. That would > also remove the need for any copy_from_user()s and the memset can be > dropped by just doing > > struct seccomp_notif unotif = {}; This doesn't zero the padding according to the C standard, so no, you can't drop the memset, or you may leak kernel stack bits. As for the rest of it, while it is an ABI change I think all of the users are CC'd on this thread, and it's an obvious goof on my part :). So: Acked-by: Tycho Andersen <tycho@tycho.ws> Tycho ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] seccomp: Check flags on seccomp_notif is unset 2019-12-26 15:37 ` Tycho Andersen @ 2019-12-27 2:28 ` Aleksa Sarai 0 siblings, 0 replies; 13+ messages in thread From: Aleksa Sarai @ 2019-12-27 2:28 UTC (permalink / raw) To: Tycho Andersen Cc: Christian Brauner, Sargun Dhillon, linux-kernel, linux-api, jannh, keescook [-- Attachment #1: Type: text/plain, Size: 3248 bytes --] On 2019-12-26, Tycho Andersen <tycho@tycho.ws> wrote: > On Fri, Dec 27, 2019 at 01:32:29AM +1100, Aleksa Sarai wrote: > > On 2019-12-26, Christian Brauner <christian.brauner@ubuntu.com> wrote: > > > On Wed, Dec 25, 2019 at 09:45:33PM +0000, Sargun Dhillon wrote: > > > > This patch is a small change in enforcement of the uapi for > > > > SECCOMP_IOCTL_NOTIF_RECV ioctl. Specificaly, the datastructure which is > > > > passed (seccomp_notif), has a flags member. Previously that could be > > > > set to a nonsense value, and we would ignore it. This ensures that > > > > no flags are set. > > > > > > > > Signed-off-by: Sargun Dhillon <sargun@sargun.me> > > > > Cc: Kees Cook <keescook@chromium.org> > > > > > > I'm fine with this since we soon want to make use of the flag argument > > > when we add a flag to get a pidfd from the seccomp notifier on receive. > > > The major users I could identify already pass in seccomp_notif with all > > > fields set to 0. If we really break users we can always revert; this > > > seems very unlikely to me though. > > > > > > One more question below, otherwise: > > > > > > Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com> > > > > > > > --- > > > > kernel/seccomp.c | 7 +++++++ > > > > 1 file changed, 7 insertions(+) > > > > > > > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c > > > > index 12d2227e5786..455925557490 100644 > > > > --- a/kernel/seccomp.c > > > > +++ b/kernel/seccomp.c > > > > @@ -1026,6 +1026,13 @@ static long seccomp_notify_recv(struct seccomp_filter *filter, > > > > struct seccomp_notif unotif; > > > > ssize_t ret; > > > > > > > > + if (copy_from_user(&unotif, buf, sizeof(unotif))) > > > > + return -EFAULT; > > > > + > > > > + /* flags is reserved right now, make sure it's unset */ > > > > + if (unotif.flags) > > > > + return -EINVAL; > > > > + > > > > > > Might it make sense to use > > > > > > err = copy_struct_from_user(&unotif, sizeof(unotif), buf, sizeof(unotif)); > > > if (err) > > > return err; > > > > > > This way we check that the whole struct is 0 and report an error as soon > > > as one of the members is non-zero. That's more drastic but it'd ensure > > > that other fields can be used in the future for whatever purposes. > > > It would also let us get rid of the memset() below. > > > > Given that this isn't an extensible struct, it would be simpler to just do > > check_zeroed_user() -- copy_struct_from_user() is overkill. That would > > also remove the need for any copy_from_user()s and the memset can be > > dropped by just doing > > > > struct seccomp_notif unotif = {}; > > This doesn't zero the padding according to the C standard, so no, you > can't drop the memset, or you may leak kernel stack bits. Ah right, I didn't double-check if there was any un-named. IMHO, It's a bit odd to have un-named padding in a struct intended for extensions (specifically to avoid these problems -- because it means userspace will pass garbage by accident and there's nothing we can do about it). But it's a bit late to worry about that now. :P -- Aleksa Sarai Senior Software Engineer (Containers) SUSE Linux GmbH <https://www.cyphar.com/> [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-12-27 15:32 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-25 21:45 [PATCH] seccomp: Check flags on seccomp_notif is unset Sargun Dhillon 2019-12-26 11:52 ` Christian Brauner 2019-12-26 14:32 ` Aleksa Sarai 2019-12-26 14:34 ` Christian Brauner 2019-12-27 2:24 ` Aleksa Sarai 2019-12-27 2:31 ` Aleksa Sarai 2019-12-27 11:47 ` Christian Brauner 2019-12-27 14:22 ` Sargun Dhillon 2019-12-27 14:38 ` Tycho Andersen 2019-12-27 15:15 ` Aleksa Sarai 2019-12-27 15:32 ` Christian Brauner 2019-12-26 15:37 ` Tycho Andersen 2019-12-27 2:28 ` Aleksa Sarai
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).