[PATCH v2 1/2] samples, selftests/seccomp: Zero out seccomp_notif

[PATCH v2 1/2] samples, selftests/seccomp: Zero out seccomp_notif

[Sargun Dhillon]

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 <sargun@sargun.me>
Cc: Kees Cook <keescook@chromium.org>
---
 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

Re: [PATCH v2 1/2] samples, selftests/seccomp: Zero out seccomp_notif

[Christian Brauner]

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 <sargun@sargun.me>
> Cc: Kees Cook <keescook@chromium.org>

Can you please also add a test, that verifies that we catch garbage
values, please?

Re: [PATCH v2 1/2] samples, selftests/seccomp: Zero out seccomp_notif

[Tycho Andersen]

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 <sargun@sargun.me>
> Cc: Kees Cook <keescook@chromium.org>
> ---
>  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 <tycho@tycho.ws>

Cheers,

Tycho

Re: [PATCH v2 1/2] samples, selftests/seccomp: Zero out seccomp_notif

[Sargun Dhillon]

On Sat, Dec 28, 2019 at 1:18 PM Tycho Andersen <tycho@tycho.ws> 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 <sargun@sargun.me>
> > Cc: Kees Cook <keescook@chromium.org>
> > ---
> >  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 <tycho@tycho.ws>
>
> Cheers,
>
> Tycho

Re: [PATCH v2 1/2] samples, selftests/seccomp: Zero out seccomp_notif

[Tycho Andersen]

On Sat, Dec 28, 2019 at 07:10:29PM -0500, Sargun Dhillon wrote:
> On Sat, Dec 28, 2019 at 1:18 PM Tycho Andersen <tycho@tycho.ws> 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 <sargun@sargun.me>
> > > Cc: Kees Cook <keescook@chromium.org>
> > > ---
> > >  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

Re: [PATCH v2 1/2] samples, selftests/seccomp: Zero out seccomp_notif

[Sargun Dhillon]

On Sat, Dec 28, 2019 at 4:18 PM Tycho Andersen <tycho@tycho.ws> 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 <tycho@tycho.ws> 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?

Re: [PATCH v2 1/2] samples, selftests/seccomp: Zero out seccomp_notif

[Kees Cook]

On Mon, Dec 30, 2019 at 11:14:44AM -0800, Sargun Dhillon wrote:
> On Sat, Dec 28, 2019 at 4:18 PM Tycho Andersen <tycho@tycho.ws> 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 <tycho@tycho.ws> 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