* [PATCH] net: filter: fix upper BPF instruction limit
@ 2014-06-18 22:34 Kees Cook
2014-06-18 22:43 ` Daniel Borkmann
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Kees Cook @ 2014-06-18 22:34 UTC (permalink / raw)
To: linux-kernel
Cc: David S. Miller, Alexei Starovoitov, Daniel Borkmann,
Eric Dumazet, Chema Gonzalez, netdev
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] net: filter: fix upper BPF instruction limit
2014-06-18 22:34 [PATCH] net: filter: fix upper BPF instruction limit Kees Cook
@ 2014-06-18 22:43 ` Daniel Borkmann
2014-06-18 22:48 ` Alexei Starovoitov
2014-06-19 0:05 ` David Miller
2 siblings, 0 replies; 10+ messages in thread
From: Daniel Borkmann @ 2014-06-18 22:43 UTC (permalink / raw)
To: Kees Cook
Cc: linux-kernel, David S. Miller, Alexei Starovoitov, Eric Dumazet,
Chema Gonzalez, netdev
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>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] net: filter: fix upper BPF instruction limit
2014-06-18 22:34 [PATCH] net: filter: fix upper BPF instruction limit Kees Cook
2014-06-18 22:43 ` Daniel Borkmann
@ 2014-06-18 22:48 ` Alexei Starovoitov
2014-06-18 22:55 ` Kees Cook
2014-06-19 0:05 ` David Miller
2 siblings, 1 reply; 10+ messages in thread
From: Alexei Starovoitov @ 2014-06-18 22:48 UTC (permalink / raw)
To: Kees Cook
Cc: LKML, David S. Miller, Daniel Borkmann, Eric Dumazet,
Chema Gonzalez, Network Development
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!
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] net: filter: fix upper BPF instruction limit
2014-06-18 22:48 ` Alexei Starovoitov
@ 2014-06-18 22:55 ` Kees Cook
2014-06-18 23:19 ` Alexei Starovoitov
0 siblings, 1 reply; 10+ messages in thread
From: Kees Cook @ 2014-06-18 22:55 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: LKML, David S. Miller, Daniel Borkmann, Eric Dumazet,
Chema Gonzalez, Network Development
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] net: filter: fix upper BPF instruction limit
2014-06-18 22:55 ` Kees Cook
@ 2014-06-18 23:19 ` Alexei Starovoitov
2014-06-18 23:28 ` Kees Cook
0 siblings, 1 reply; 10+ messages in thread
From: Alexei Starovoitov @ 2014-06-18 23:19 UTC (permalink / raw)
To: Kees Cook
Cc: LKML, David S. Miller, Daniel Borkmann, Eric Dumazet,
Chema Gonzalez, Network Development
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?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] net: filter: fix upper BPF instruction limit
2014-06-18 23:19 ` Alexei Starovoitov
@ 2014-06-18 23:28 ` Kees Cook
2014-06-20 10:13 ` Daniel Borkmann
0 siblings, 1 reply; 10+ messages in thread
From: Kees Cook @ 2014-06-18 23:28 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: LKML, David S. Miller, Daniel Borkmann, Eric Dumazet,
Chema Gonzalez, Network Development
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] net: filter: fix upper BPF instruction limit
2014-06-18 22:34 [PATCH] net: filter: fix upper BPF instruction limit Kees Cook
2014-06-18 22:43 ` Daniel Borkmann
2014-06-18 22:48 ` Alexei Starovoitov
@ 2014-06-19 0:05 ` David Miller
2 siblings, 0 replies; 10+ messages in thread
From: David Miller @ 2014-06-19 0:05 UTC (permalink / raw)
To: keescook; +Cc: linux-kernel, ast, dborkman, edumazet, chema, netdev
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] net: filter: fix upper BPF instruction limit
2014-06-18 23:28 ` Kees Cook
@ 2014-06-20 10:13 ` Daniel Borkmann
2014-06-20 16:48 ` Kees Cook
0 siblings, 1 reply; 10+ messages in thread
From: Daniel Borkmann @ 2014-06-20 10:13 UTC (permalink / raw)
To: Kees Cook
Cc: Alexei Starovoitov, LKML, David S. Miller, Eric Dumazet,
Chema Gonzalez, Network Development
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] net: filter: fix upper BPF instruction limit
2014-06-20 10:13 ` Daniel Borkmann
@ 2014-06-20 16:48 ` Kees Cook
2014-06-20 21:00 ` Daniel Borkmann
0 siblings, 1 reply; 10+ messages in thread
From: Kees Cook @ 2014-06-20 16:48 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Alexei Starovoitov, LKML, David S. Miller, Eric Dumazet,
Chema Gonzalez, Network Development
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] net: filter: fix upper BPF instruction limit
2014-06-20 16:48 ` Kees Cook
@ 2014-06-20 21:00 ` Daniel Borkmann
0 siblings, 0 replies; 10+ messages in thread
From: Daniel Borkmann @ 2014-06-20 21:00 UTC (permalink / raw)
To: Kees Cook
Cc: Alexei Starovoitov, LKML, David S. Miller, Eric Dumazet,
Chema Gonzalez, Network Development
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-06-20 21:00 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-06-18 22:34 [PATCH] net: filter: fix upper BPF instruction limit Kees Cook
2014-06-18 22:43 ` Daniel Borkmann
2014-06-18 22:48 ` Alexei Starovoitov
2014-06-18 22:55 ` Kees Cook
2014-06-18 23:19 ` Alexei Starovoitov
2014-06-18 23:28 ` Kees Cook
2014-06-20 10:13 ` Daniel Borkmann
2014-06-20 16:48 ` Kees Cook
2014-06-20 21:00 ` Daniel Borkmann
2014-06-19 0:05 ` David Miller
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).