The seccomp_notif structure should be zeroed out prior to calling the
SECCOMP_IOCTL_NOTIF_RECV ioctl. Previously, the kernel did not check
whether these structures were zeroed out or not, so these worked.
Signed-off-by: Sargun Dhillon <[email protected]>
Cc: Kees Cook <[email protected]>
---
samples/seccomp/user-trap.c | 2 +-
tools/testing/selftests/seccomp/seccomp_bpf.c | 2 ++
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/samples/seccomp/user-trap.c b/samples/seccomp/user-trap.c
index 6d0125ca8af7..0ca8fb37cd79 100644
--- a/samples/seccomp/user-trap.c
+++ b/samples/seccomp/user-trap.c
@@ -298,7 +298,6 @@ int main(void)
req = malloc(sizes.seccomp_notif);
if (!req)
goto out_close;
- memset(req, 0, sizeof(*req));
resp = malloc(sizes.seccomp_notif_resp);
if (!resp)
@@ -306,6 +305,7 @@ int main(void)
memset(resp, 0, sizeof(*resp));
while (1) {
+ memset(req, 0, sizes.seccomp_notif);
if (ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, req)) {
perror("ioctl recv");
goto out_resp;
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 6944b898bb53..f53f14971bff 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -3278,6 +3278,7 @@ TEST(user_notification_signal)
close(sk_pair[1]);
+ memset(&req, 0, sizeof(req));
EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
EXPECT_EQ(kill(pid, SIGUSR1), 0);
@@ -3296,6 +3297,7 @@ TEST(user_notification_signal)
EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), -1);
EXPECT_EQ(errno, ENOENT);
+ memset(&req, 0, sizeof(req));
EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
resp.id = req.id;
--
2.20.1
On Sat, Dec 28, 2019 at 01:48:39AM +0000, Sargun Dhillon wrote:
> The seccomp_notif structure should be zeroed out prior to calling the
> SECCOMP_IOCTL_NOTIF_RECV ioctl. Previously, the kernel did not check
> whether these structures were zeroed out or not, so these worked.
>
> Signed-off-by: Sargun Dhillon <[email protected]>
> Cc: Kees Cook <[email protected]>
Can you please also add a test, that verifies that we catch garbage
values, please?
On Sat, Dec 28, 2019 at 01:48:39AM +0000, Sargun Dhillon wrote:
> The seccomp_notif structure should be zeroed out prior to calling the
> SECCOMP_IOCTL_NOTIF_RECV ioctl. Previously, the kernel did not check
> whether these structures were zeroed out or not, so these worked.
>
> Signed-off-by: Sargun Dhillon <[email protected]>
> Cc: Kees Cook <[email protected]>
> ---
> samples/seccomp/user-trap.c | 2 +-
> tools/testing/selftests/seccomp/seccomp_bpf.c | 2 ++
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/samples/seccomp/user-trap.c b/samples/seccomp/user-trap.c
> index 6d0125ca8af7..0ca8fb37cd79 100644
> --- a/samples/seccomp/user-trap.c
> +++ b/samples/seccomp/user-trap.c
> @@ -298,7 +298,6 @@ int main(void)
> req = malloc(sizes.seccomp_notif);
> if (!req)
> goto out_close;
> - memset(req, 0, sizeof(*req));
>
> resp = malloc(sizes.seccomp_notif_resp);
> if (!resp)
> @@ -306,6 +305,7 @@ int main(void)
> memset(resp, 0, sizeof(*resp));
I know it's unrelated, but it's probably worth sending a patch to fix
this to be sizes.seccomp_notif_resp instead of sizeof(*resp), since if
the kernel is older this will over-zero things. I can do that, or you
can add the patch to this series, just let me know which.
But in any case, this patch is:
Reviewed-by: Tycho Andersen <[email protected]>
Cheers,
Tycho
On Sat, Dec 28, 2019 at 1:18 PM Tycho Andersen <[email protected]> wrote:
>
> On Sat, Dec 28, 2019 at 01:48:39AM +0000, Sargun Dhillon wrote:
> > The seccomp_notif structure should be zeroed out prior to calling the
> > SECCOMP_IOCTL_NOTIF_RECV ioctl. Previously, the kernel did not check
> > whether these structures were zeroed out or not, so these worked.
> >
> > Signed-off-by: Sargun Dhillon <[email protected]>
> > Cc: Kees Cook <[email protected]>
> > ---
> > samples/seccomp/user-trap.c | 2 +-
> > tools/testing/selftests/seccomp/seccomp_bpf.c | 2 ++
> > 2 files changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/samples/seccomp/user-trap.c b/samples/seccomp/user-trap.c
> > index 6d0125ca8af7..0ca8fb37cd79 100644
> > --- a/samples/seccomp/user-trap.c
> > +++ b/samples/seccomp/user-trap.c
> > @@ -298,7 +298,6 @@ int main(void)
> > req = malloc(sizes.seccomp_notif);
> > if (!req)
> > goto out_close;
> > - memset(req, 0, sizeof(*req));
> >
> > resp = malloc(sizes.seccomp_notif_resp);
> > if (!resp)
> > @@ -306,6 +305,7 @@ int main(void)
> > memset(resp, 0, sizeof(*resp));
>
> I know it's unrelated, but it's probably worth sending a patch to fix
> this to be sizes.seccomp_notif_resp instead of sizeof(*resp), since if
> the kernel is older this will over-zero things. I can do that, or you
> can add the patch to this series, just let me know which.
I was thinking about this, and initially, I chose to make the smaller
change. I think it might make more sense to combine the patch,
given that the memset behaviour is "incorrect" if we do it based on
sizeof(*req), or sizeof(*resp).
I'll go ahead and respin this patch with the change to call memset
based on sizes.
>
> But in any case, this patch is:
>
> Reviewed-by: Tycho Andersen <[email protected]>
>
> Cheers,
>
> Tycho
On Sat, Dec 28, 2019 at 07:10:29PM -0500, Sargun Dhillon wrote:
> On Sat, Dec 28, 2019 at 1:18 PM Tycho Andersen <[email protected]> wrote:
> >
> > On Sat, Dec 28, 2019 at 01:48:39AM +0000, Sargun Dhillon wrote:
> > > The seccomp_notif structure should be zeroed out prior to calling the
> > > SECCOMP_IOCTL_NOTIF_RECV ioctl. Previously, the kernel did not check
> > > whether these structures were zeroed out or not, so these worked.
> > >
> > > Signed-off-by: Sargun Dhillon <[email protected]>
> > > Cc: Kees Cook <[email protected]>
> > > ---
> > > samples/seccomp/user-trap.c | 2 +-
> > > tools/testing/selftests/seccomp/seccomp_bpf.c | 2 ++
> > > 2 files changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/samples/seccomp/user-trap.c b/samples/seccomp/user-trap.c
> > > index 6d0125ca8af7..0ca8fb37cd79 100644
> > > --- a/samples/seccomp/user-trap.c
> > > +++ b/samples/seccomp/user-trap.c
> > > @@ -298,7 +298,6 @@ int main(void)
> > > req = malloc(sizes.seccomp_notif);
> > > if (!req)
> > > goto out_close;
> > > - memset(req, 0, sizeof(*req));
> > >
> > > resp = malloc(sizes.seccomp_notif_resp);
> > > if (!resp)
> > > @@ -306,6 +305,7 @@ int main(void)
> > > memset(resp, 0, sizeof(*resp));
> >
> > I know it's unrelated, but it's probably worth sending a patch to fix
> > this to be sizes.seccomp_notif_resp instead of sizeof(*resp), since if
> > the kernel is older this will over-zero things. I can do that, or you
> > can add the patch to this series, just let me know which.
>
> I was thinking about this, and initially, I chose to make the smaller
> change. I think it might make more sense to combine the patch,
> given that the memset behaviour is "incorrect" if we do it based on
> sizeof(*req), or sizeof(*resp).
>
> I'll go ahead and respin this patch with the change to call memset
> based on sizes.
I think it would be good to keep it as a separate patch, since it's an
unrelated bug fix. That way if we have to revert these because of some
breakage, we won't lose the fix.
Cheers,
Tycho
On Sat, Dec 28, 2019 at 4:18 PM Tycho Andersen <[email protected]> wrote:
>
> On Sat, Dec 28, 2019 at 07:10:29PM -0500, Sargun Dhillon wrote:
> > On Sat, Dec 28, 2019 at 1:18 PM Tycho Andersen <[email protected]> wrote:
> > >
> > >
> > > I know it's unrelated, but it's probably worth sending a patch to fix
> > > this to be sizes.seccomp_notif_resp instead of sizeof(*resp), since if
> > > the kernel is older this will over-zero things. I can do that, or you
> > > can add the patch to this series, just let me know which.
> >
> > I was thinking about this, and initially, I chose to make the smaller
> > change. I think it might make more sense to combine the patch,
> > given that the memset behaviour is "incorrect" if we do it based on
> > sizeof(*req), or sizeof(*resp).
> >
> > I'll go ahead and respin this patch with the change to call memset
> > based on sizes.
>
> I think it would be good to keep it as a separate patch, since it's an
> unrelated bug fix. That way if we have to revert these because of some
> breakage, we won't lose the fix.
>
> Cheers,
>
> Tycho
As I was doing this, I noticed that the self-tests all use hard-coded struct
sizes. When I was playing with extending the API, all of a sudden all the
self-tests started failing (until I recompiled them against newer headers).
Should we also change the self-tests to operate against the seccomp
sizes API, or was it intentional for the self-tests hard-coded the struct
definitions, and locked to the kernel version?
On Mon, Dec 30, 2019 at 11:14:44AM -0800, Sargun Dhillon wrote:
> On Sat, Dec 28, 2019 at 4:18 PM Tycho Andersen <[email protected]> wrote:
> >
> > On Sat, Dec 28, 2019 at 07:10:29PM -0500, Sargun Dhillon wrote:
> > > On Sat, Dec 28, 2019 at 1:18 PM Tycho Andersen <[email protected]> wrote:
> > > >
> > > >
> > > > I know it's unrelated, but it's probably worth sending a patch to fix
> > > > this to be sizes.seccomp_notif_resp instead of sizeof(*resp), since if
> > > > the kernel is older this will over-zero things. I can do that, or you
> > > > can add the patch to this series, just let me know which.
> > >
> > > I was thinking about this, and initially, I chose to make the smaller
> > > change. I think it might make more sense to combine the patch,
> > > given that the memset behaviour is "incorrect" if we do it based on
> > > sizeof(*req), or sizeof(*resp).
> > >
> > > I'll go ahead and respin this patch with the change to call memset
> > > based on sizes.
> >
> > I think it would be good to keep it as a separate patch, since it's an
> > unrelated bug fix. That way if we have to revert these because of some
> > breakage, we won't lose the fix.
> >
> > Cheers,
> >
> > Tycho
>
> As I was doing this, I noticed that the self-tests all use hard-coded struct
> sizes. When I was playing with extending the API, all of a sudden all the
> self-tests started failing (until I recompiled them against newer headers).
>
> Should we also change the self-tests to operate against the seccomp
> sizes API, or was it intentional for the self-tests hard-coded the struct
> definitions, and locked to the kernel version?
I intend the seccomp selftests to be kernel-version tied, but I'd like
them to fail as gracefully as possible on mismatched kernel versions...
--
Kees Cook