[PATCH] net: filter: fix upper BPF instruction limit

[PATCH] net: filter: fix upper BPF instruction limit

[Kees Cook]

The original checks (via sk_chk_filter) for instruction count uses ">",
not ">=", so changing this in sk_convert_filter has the potential to break
existing seccomp filters that used exactly BPF_MAXINSNS many instructions.

Fixes: bd4cf0ed331a ("net: filter: rework/optimize internal BPF interpreter's instruction set")
Signed-off-by: Kees Cook <keescook@chromium.org>
Cc: stable@vger.kernel.org # v3.15+
---
 net/core/filter.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 735fad897496..a44e12cdde4c 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -840,7 +840,7 @@ int sk_convert_filter(struct sock_filter *prog, int len,
 	BUILD_BUG_ON(BPF_MEMWORDS * sizeof(u32) > MAX_BPF_STACK);
 	BUILD_BUG_ON(BPF_REG_FP + 1 != MAX_BPF_REG);
 
-	if (len <= 0 || len >= BPF_MAXINSNS)
+	if (len <= 0 || len > BPF_MAXINSNS)
 		return -EINVAL;
 
 	if (new_prog) {
-- 
1.7.9.5


-- 
Kees Cook
Chrome OS Security

Re: [PATCH] net: filter: fix upper BPF instruction limit

[Daniel Borkmann]

On 06/19/2014 12:34 AM, Kees Cook wrote:
> The original checks (via sk_chk_filter) for instruction count uses ">",
> not ">=", so changing this in sk_convert_filter has the potential to break
> existing seccomp filters that used exactly BPF_MAXINSNS many instructions.
>
> Fixes: bd4cf0ed331a ("net: filter: rework/optimize internal BPF interpreter's instruction set")
> Signed-off-by: Kees Cook <keescook@chromium.org>

Acked-by: Daniel Borkmann <dborkman@redhat.com>

Re: [PATCH] net: filter: fix upper BPF instruction limit

[Alexei Starovoitov]

On Wed, Jun 18, 2014 at 3:34 PM, Kees Cook <keescook@chromium.org> wrote:
> The original checks (via sk_chk_filter) for instruction count uses ">",
> not ">=", so changing this in sk_convert_filter has the potential to break
> existing seccomp filters that used exactly BPF_MAXINSNS many instructions.
>
> Fixes: bd4cf0ed331a ("net: filter: rework/optimize internal BPF interpreter's instruction set")
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: stable@vger.kernel.org # v3.15+

Acked-by: Alexei Starovoitov <ast@plumgrid.com>

I wonder how did you catch this? :)
Just code inspection or seccomp actually generating such programs?

Thanks!

Re: [PATCH] net: filter: fix upper BPF instruction limit

[Kees Cook]

On Wed, Jun 18, 2014 at 3:48 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
> On Wed, Jun 18, 2014 at 3:34 PM, Kees Cook <keescook@chromium.org> wrote:
>> The original checks (via sk_chk_filter) for instruction count uses ">",
>> not ">=", so changing this in sk_convert_filter has the potential to break
>> existing seccomp filters that used exactly BPF_MAXINSNS many instructions.
>>
>> Fixes: bd4cf0ed331a ("net: filter: rework/optimize internal BPF interpreter's instruction set")
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> Cc: stable@vger.kernel.org # v3.15+
>
> Acked-by: Alexei Starovoitov <ast@plumgrid.com>
>
> I wonder how did you catch this? :)
> Just code inspection or seccomp actually generating such programs?

In the process of merging my seccomp thread-sync series back with
mainline, I got uncomfortable that I was moving filter size validation
around without actually testing it. When I added it, I was happy that
my series was correctly checking size limits, but then discovered my
newly added check actually failed on an earlier kernel (3.2). Tracking
it down found the corner case under 3.15.

Here's the test I added to the seccomp regression tests, if you're interested:
https://github.com/kees/seccomp/commit/794d54a340cde70a3bdf7fe0ade1f95d160b2883

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH] net: filter: fix upper BPF instruction limit

[Alexei Starovoitov]

On Wed, Jun 18, 2014 at 3:55 PM, Kees Cook <keescook@chromium.org> wrote:
> On Wed, Jun 18, 2014 at 3:48 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
>> On Wed, Jun 18, 2014 at 3:34 PM, Kees Cook <keescook@chromium.org> wrote:
>>> The original checks (via sk_chk_filter) for instruction count uses ">",
>>> not ">=", so changing this in sk_convert_filter has the potential to break
>>> existing seccomp filters that used exactly BPF_MAXINSNS many instructions.
>>>
>>> Fixes: bd4cf0ed331a ("net: filter: rework/optimize internal BPF interpreter's instruction set")
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>> Cc: stable@vger.kernel.org # v3.15+
>>
>> Acked-by: Alexei Starovoitov <ast@plumgrid.com>
>>
>> I wonder how did you catch this? :)
>> Just code inspection or seccomp actually generating such programs?
>
> In the process of merging my seccomp thread-sync series back with
> mainline, I got uncomfortable that I was moving filter size validation
> around without actually testing it. When I added it, I was happy that
> my series was correctly checking size limits, but then discovered my
> newly added check actually failed on an earlier kernel (3.2). Tracking
> it down found the corner case under 3.15.
>
> Here's the test I added to the seccomp regression tests, if you're interested:
> https://github.com/kees/seccomp/commit/794d54a340cde70a3bdf7fe0ade1f95d160b2883

Nice. I'm assuming https://github.com/redpig/seccomp is still the main tree
for seccomp testsuite…

btw I've tried to add 'real' test to it (one generated by chrome)

+TEST(chrome_syscalls) {
+       static struct sock_filter filter[] = {
+               { 32, 240, 61, 4 },          /* 0:      ld [4] */
+               { 21, 1, 0, -1073741762 },   /* 1:      jeq #0xc000003e, 3, 2 */
+               { 5, 0, 0, 271 },            /* 2:      ja 274 */
+               { 32, 208, 198, 0 },         /* 3:      ld [0] */
+               { 69, 0, 1, 1073741824 },    /* 4:      jset
#0x40000000, 5, 6 */
+               { 6, 0, 0, 196615 },         /* 5:      ret #0x30007 */
+               { 53, 0, 7, 121 },           /* 6:      jge #0x79, 7, 14 */
+               { 53, 0, 12, 214 },          /* 7:      jge #0xd6, 8, 20 */
…
+               { 6, 0, 0, 2147418112 },     /* 272:    ret #0x7fff0000 */
+               { 6, 0, 0, 327681 },         /* 273:    ret #0x50001 */
+               { 6, 0, 0, 196610 },         /* 274:    ret #0x30002 */
+       };
...
+       for (i = 0; i < MAX_SYSCALLS; i++) {
+               ch_pid = fork();
+               ASSERT_LE(0, ch_pid);
+
+               if (ch_pid == 0) {
+                       ret = prctl(PR_SET_SECCOMP,
+                              SECCOMP_MODE_FILTER, &prog);
+                       ASSERT_EQ(0, ret);
+#define MAGIC (-1ll << 2)
+                       err = syscall(i, MAGIC, MAGIC, MAGIC,
+                              MAGIC, MAGIC, MAGIC);
+                       syscall(__NR_exit, 0);
+               }
+               wait(&status);
+               if (status != expected_status[i])
…

but it's really x64 only and looks ugly. Do you have better ideas
on how to test all possible paths through auto-generated branch tree?

Re: [PATCH] net: filter: fix upper BPF instruction limit

[Kees Cook]

On Wed, Jun 18, 2014 at 4:19 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
> On Wed, Jun 18, 2014 at 3:55 PM, Kees Cook <keescook@chromium.org> wrote:
>> On Wed, Jun 18, 2014 at 3:48 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
>>> On Wed, Jun 18, 2014 at 3:34 PM, Kees Cook <keescook@chromium.org> wrote:
>>>> The original checks (via sk_chk_filter) for instruction count uses ">",
>>>> not ">=", so changing this in sk_convert_filter has the potential to break
>>>> existing seccomp filters that used exactly BPF_MAXINSNS many instructions.
>>>>
>>>> Fixes: bd4cf0ed331a ("net: filter: rework/optimize internal BPF interpreter's instruction set")
>>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>>> Cc: stable@vger.kernel.org # v3.15+
>>>
>>> Acked-by: Alexei Starovoitov <ast@plumgrid.com>
>>>
>>> I wonder how did you catch this? :)
>>> Just code inspection or seccomp actually generating such programs?
>>
>> In the process of merging my seccomp thread-sync series back with
>> mainline, I got uncomfortable that I was moving filter size validation
>> around without actually testing it. When I added it, I was happy that
>> my series was correctly checking size limits, but then discovered my
>> newly added check actually failed on an earlier kernel (3.2). Tracking
>> it down found the corner case under 3.15.
>>
>> Here's the test I added to the seccomp regression tests, if you're interested:
>> https://github.com/kees/seccomp/commit/794d54a340cde70a3bdf7fe0ade1f95d160b2883
>
> Nice. I'm assuming https://github.com/redpig/seccomp is still the main tree
> for seccomp testsuite…

Yes. Will hasn't pulled this most recent set of changes.

>
> btw I've tried to add 'real' test to it (one generated by chrome)
>
> +TEST(chrome_syscalls) {
> +       static struct sock_filter filter[] = {
> +               { 32, 240, 61, 4 },          /* 0:      ld [4] */
> +               { 21, 1, 0, -1073741762 },   /* 1:      jeq #0xc000003e, 3, 2 */
> +               { 5, 0, 0, 271 },            /* 2:      ja 274 */
> +               { 32, 208, 198, 0 },         /* 3:      ld [0] */
> +               { 69, 0, 1, 1073741824 },    /* 4:      jset
> #0x40000000, 5, 6 */
> +               { 6, 0, 0, 196615 },         /* 5:      ret #0x30007 */
> +               { 53, 0, 7, 121 },           /* 6:      jge #0x79, 7, 14 */
> +               { 53, 0, 12, 214 },          /* 7:      jge #0xd6, 8, 20 */
> …
> +               { 6, 0, 0, 2147418112 },     /* 272:    ret #0x7fff0000 */
> +               { 6, 0, 0, 327681 },         /* 273:    ret #0x50001 */
> +               { 6, 0, 0, 196610 },         /* 274:    ret #0x30002 */
> +       };
> ...
> +       for (i = 0; i < MAX_SYSCALLS; i++) {
> +               ch_pid = fork();
> +               ASSERT_LE(0, ch_pid);
> +
> +               if (ch_pid == 0) {
> +                       ret = prctl(PR_SET_SECCOMP,
> +                              SECCOMP_MODE_FILTER, &prog);
> +                       ASSERT_EQ(0, ret);
> +#define MAGIC (-1ll << 2)
> +                       err = syscall(i, MAGIC, MAGIC, MAGIC,
> +                              MAGIC, MAGIC, MAGIC);
> +                       syscall(__NR_exit, 0);
> +               }
> +               wait(&status);
> +               if (status != expected_status[i])
> …
>
> but it's really x64 only and looks ugly. Do you have better ideas
> on how to test all possible paths through auto-generated branch tree?

For testing BPF application filter logic itself, I lean on the test
suite in libseccomp, which does a ton of filter generation and
testing. The seccomp regression tests in the github tree tend to focus
on validating the basic behavioral elements (kill, trace, trap, etc).

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH] net: filter: fix upper BPF instruction limit

[Daniel Borkmann]

Hi Kees,

On 06/19/2014 01:28 AM, Kees Cook wrote:
> On Wed, Jun 18, 2014 at 4:19 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
>> On Wed, Jun 18, 2014 at 3:55 PM, Kees Cook <keescook@chromium.org> wrote:
>>> On Wed, Jun 18, 2014 at 3:48 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
>>>> On Wed, Jun 18, 2014 at 3:34 PM, Kees Cook <keescook@chromium.org> wrote:
...
>>>> I wonder how did you catch this? :)
>>>> Just code inspection or seccomp actually generating such programs?
>>>
>>> In the process of merging my seccomp thread-sync series back with
>>> mainline, I got uncomfortable that I was moving filter size validation
>>> around without actually testing it. When I added it, I was happy that
>>> my series was correctly checking size limits, but then discovered my
>>> newly added check actually failed on an earlier kernel (3.2). Tracking
>>> it down found the corner case under 3.15.
>>>
>>> Here's the test I added to the seccomp regression tests, if you're interested:
>>> https://github.com/kees/seccomp/commit/794d54a340cde70a3bdf7fe0ade1f95d160b2883
>>
>> Nice. I'm assuming https://github.com/redpig/seccomp is still the main tree
>> for seccomp testsuite…
>
> Yes. Will hasn't pulled this most recent set of changes.

We were actually thinking about extending lib/test_bpf module with seccomp
tests, which is possible to a limited extend, but seccomp is also a bit
more than just running a BPF program and making sure results fit.

Are there any plans to put and extend test cases from [1] via user space
side into the kernel self-test directory, i.e. into something like
tools/testing/selftests/seccomp/ so that in future new tests can be added
or run from there? Might be worth to consider.

Thanks,

Daniel

  [1] https://github.com/redpig/seccomp

Re: [PATCH] net: filter: fix upper BPF instruction limit

[Kees Cook]

On Fri, Jun 20, 2014 at 3:13 AM, Daniel Borkmann <dborkman@redhat.com> wrote:
> Hi Kees,
>
>
> On 06/19/2014 01:28 AM, Kees Cook wrote:
>>
>> On Wed, Jun 18, 2014 at 4:19 PM, Alexei Starovoitov <ast@plumgrid.com>
>> wrote:
>>>
>>> On Wed, Jun 18, 2014 at 3:55 PM, Kees Cook <keescook@chromium.org> wrote:
>>>>
>>>> On Wed, Jun 18, 2014 at 3:48 PM, Alexei Starovoitov <ast@plumgrid.com>
>>>> wrote:
>>>>>
>>>>> On Wed, Jun 18, 2014 at 3:34 PM, Kees Cook <keescook@chromium.org>
>>>>> wrote:
>
> ...
>
>>>>> I wonder how did you catch this? :)
>>>>> Just code inspection or seccomp actually generating such programs?
>>>>
>>>>
>>>> In the process of merging my seccomp thread-sync series back with
>>>> mainline, I got uncomfortable that I was moving filter size validation
>>>> around without actually testing it. When I added it, I was happy that
>>>> my series was correctly checking size limits, but then discovered my
>>>> newly added check actually failed on an earlier kernel (3.2). Tracking
>>>> it down found the corner case under 3.15.
>>>>
>>>> Here's the test I added to the seccomp regression tests, if you're
>>>> interested:
>>>>
>>>> https://github.com/kees/seccomp/commit/794d54a340cde70a3bdf7fe0ade1f95d160b2883
>>>
>>>
>>> Nice. I'm assuming https://github.com/redpig/seccomp is still the main
>>> tree
>>> for seccomp testsuite…
>>
>>
>> Yes. Will hasn't pulled this most recent set of changes.
>
>
> We were actually thinking about extending lib/test_bpf module with seccomp
> tests, which is possible to a limited extend, but seccomp is also a bit
> more than just running a BPF program and making sure results fit.
>
> Are there any plans to put and extend test cases from [1] via user space
> side into the kernel self-test directory, i.e. into something like
> tools/testing/selftests/seccomp/ so that in future new tests can be added
> or run from there? Might be worth to consider.

Yeah, I have this on my TODO list, but we need to juggle relicensing
the test suite (it is currently BSD, not GPLv2). I'll keep chasing
this.

-Kees

>
> Thanks,
>
> Daniel
>
>  [1] https://github.com/redpig/seccomp



-- 
Kees Cook
Chrome OS Security

Re: [PATCH] net: filter: fix upper BPF instruction limit

[Daniel Borkmann]

On 06/20/2014 06:48 PM, Kees Cook wrote:
...
>> Are there any plans to put and extend test cases from [1] via user space
>> side into the kernel self-test directory, i.e. into something like
>> tools/testing/selftests/seccomp/ so that in future new tests can be added
>> or run from there? Might be worth to consider.
>
> Yeah, I have this on my TODO list, but we need to juggle relicensing
> the test suite (it is currently BSD, not GPLv2). I'll keep chasing
> this.

Sounds good, please keep us in the loop.

Thanks,

Daniel

Re: [PATCH] net: filter: fix upper BPF instruction limit

[David Miller]

From: Kees Cook <keescook@chromium.org>
Date: Wed, 18 Jun 2014 15:34:57 -0700

> The original checks (via sk_chk_filter) for instruction count uses ">",
> not ">=", so changing this in sk_convert_filter has the potential to break
> existing seccomp filters that used exactly BPF_MAXINSNS many instructions.
> 
> Fixes: bd4cf0ed331a ("net: filter: rework/optimize internal BPF interpreter's instruction set")
> Signed-off-by: Kees Cook <keescook@chromium.org>

Applied, thanks.