[PATCH bpf v2 0/2] bpf: enhance validation of pointer formatting

[PATCH bpf v2 0/2] bpf: enhance validation of pointer formatting

[Ilya Shchipletsov]

This patch series enhances validation of pointer formatting to prevent same
exact issue happening again, as it happen before in [0] and happened now.

[0]: https://lkml.kernel.org/netdev/85a08645-453b-78ad-e401-55d2894fa64a@iogearbox.net/T/

Co-developed-by: Nikita Marushkin <hfggklm@gmail.com>
Signed-off-by: Nikita Marushkin <hfggklm@gmail.com>
Signed-off-by: Ilya Shchipletsov <rabbelkin@mail.ru>
---
Changes in v2:
- Added Reported-by for syzbot [Florent Revest]
- Added negative tests for snprintf [Florent Revest]
- Moved comment to first 'if' statement [Yonghong Song]
- Link to v1: https://lore.kernel.org/bpf/9679a031-3858-4fef-bb8e-1cf436696095@mail.ru/
---
Ilya Shchipletsov (2):
  bpf: fix %p% runtime check in bpf_bprintf_prepare
  selftests/bpf: Add test cases for various pointer specifiers

 kernel/bpf/helpers.c                              | 13 +++++++++----
 tools/testing/selftests/bpf/prog_tests/snprintf.c | 15 +++++++++++++++
 2 files changed, 24 insertions(+), 4 deletions(-)
-- 
2.43.0

[PATCH bpf v2 1/2] bpf: fix %p% runtime check in bpf_bprintf_prepare

[Ilya Shchipletsov]

Fuzzing reports a warning in format_decode()

Please remove unsupported %� in format string
WARNING: CPU: 0 PID: 5091 at lib/vsprintf.c:2680 format_decode+0x1193/0x1bb0 lib/vsprintf.c:2680
Modules linked in:
CPU: 0 PID: 5091 Comm: syz-executor879 Not tainted 6.10.0-rc1-syzkaller-00021-ge0cce98fe279 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 04/02/2024
RIP: 0010:format_decode+0x1193/0x1bb0 lib/vsprintf.c:2680
Call Trace:
 <TASK>
 bstr_printf+0x137/0x1210 lib/vsprintf.c:3253
 ____bpf_trace_printk kernel/trace/bpf_trace.c:390 [inline]
 bpf_trace_printk+0x1a1/0x230 kernel/trace/bpf_trace.c:375
 bpf_prog_21da1b68f62e1237+0x36/0x41
 bpf_dispatcher_nop_func include/linux/bpf.h:1243 [inline]
 __bpf_prog_run include/linux/filter.h:691 [inline]
 bpf_prog_run include/linux/filter.h:698 [inline]
 bpf_test_run+0x40b/0x910 net/bpf/test_run.c:425
 bpf_prog_test_run_skb+0xafa/0x13a0 net/bpf/test_run.c:1066
 bpf_prog_test_run+0x33c/0x3b0 kernel/bpf/syscall.c:4291
 __sys_bpf+0x48d/0x810 kernel/bpf/syscall.c:5705
 __do_sys_bpf kernel/bpf/syscall.c:5794 [inline]
 __se_sys_bpf kernel/bpf/syscall.c:5792 [inline]
 __x64_sys_bpf+0x7c/0x90 kernel/bpf/syscall.c:5792
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

The problem occurs when trying to pass %p% at the end of format string,
which would result in skipping last % and passing invalid format string
down to format_decode() that would cause warning because of invalid
character after %.

Fix issue by advancing pointer only if next char is format modifier.
If next char is null/space/punct, then just accept formatting as is,
without advancing the pointer.

Found by Linux Verification Center (linuxtesting.org) with Syzkaller.

Reported-by: syzbot+e2c932aec5c8a6e1d31c@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=e2c932aec5c8a6e1d31c
Fixes: 48cac3f4a96d ("bpf: Implement formatted output helpers with bstr_printf")
Co-developed-by: Nikita Marushkin <hfggklm@gmail.com>
Signed-off-by: Nikita Marushkin <hfggklm@gmail.com>
Signed-off-by: Ilya Shchipletsov <rabbelkin@mail.ru>
---
 kernel/bpf/helpers.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 1a43d06eab28..3efa8b04999a 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -889,10 +889,8 @@ int bpf_bprintf_prepare(char *fmt, u32 fmt_size, const u64 *raw_args,
 				goto fmt_str;
 			}
 
-			if (fmt[i + 1] == 0 || isspace(fmt[i + 1]) ||
-			    ispunct(fmt[i + 1]) || fmt[i + 1] == 'K' ||
-			    fmt[i + 1] == 'x' || fmt[i + 1] == 's' ||
-			    fmt[i + 1] == 'S') {
+			if (fmt[i + 1] == 'K' || fmt[i + 1] == 'x' ||
+			    fmt[i + 1] == 's' || fmt[i + 1] == 'S') {
 				/* just kernel pointers */
 				if (tmp_buf)
 					cur_arg = raw_args[num_spec];
@@ -900,6 +898,13 @@ int bpf_bprintf_prepare(char *fmt, u32 fmt_size, const u64 *raw_args,
 				goto nocopy_fmt;
 			}
 
+			if (fmt[i + 1] == 0 || isspace(fmt[i + 1]) ||
+			    ispunct(fmt[i + 1])) {
+				if (tmp_buf)
+					cur_arg = raw_args[num_spec];
+				goto nocopy_fmt;
+			}
+
 			if (fmt[i + 1] == 'B') {
 				if (tmp_buf)  {
 					err = snprintf(tmp_buf,
-- 
2.43.0

[PATCH bpf v2 2/2] selftests/bpf: Add test cases for various pointer specifiers

[Ilya Shchipletsov]

Extend snprintf negative tests to cover pointer specifiers to prevent possible
invalid handling of %p% from happening again.

 ./test_progs -t snprintf
 #302/1   snprintf/snprintf_positive:OK
 #302/2   snprintf/snprintf_negative:OK
 #302     snprintf:OK
 #303     snprintf_btf:OK
 Summary: 2/2 PASSED, 0 SKIPPED, 0 FAILED

Co-developed-by: Nikita Marushkin <hfggklm@gmail.com>
Signed-off-by: Nikita Marushkin <hfggklm@gmail.com>
Signed-off-by: Ilya Shchipletsov <rabbelkin@mail.ru>
---
 tools/testing/selftests/bpf/prog_tests/snprintf.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/snprintf.c b/tools/testing/selftests/bpf/prog_tests/snprintf.c
index 4be6fdb78c6a..b5b6371e09bb 100644
--- a/tools/testing/selftests/bpf/prog_tests/snprintf.c
+++ b/tools/testing/selftests/bpf/prog_tests/snprintf.c
@@ -116,6 +116,21 @@ static void test_snprintf_negative(void)
 	ASSERT_ERR(load_single_snprintf("%llc"), "invalid specifier 7");
 	ASSERT_ERR(load_single_snprintf("\x80"), "non ascii character");
 	ASSERT_ERR(load_single_snprintf("\x1"), "non printable character");
+
+	ASSERT_OK(load_single_snprintf("valid %p"), "valid usage");
+
+	ASSERT_ERR(load_single_snprintf("%p%"), "too many specifiers 1");
+	ASSERT_ERR(load_single_snprintf("%pK%"), "too many specifiers 2");
+	ASSERT_ERR(load_single_snprintf("%px%"), "too many specifiers 3");
+	ASSERT_ERR(load_single_snprintf("%ps%"), "too many specifiers 4");
+	ASSERT_ERR(load_single_snprintf("%pS%"), "too many specifiers 5");
+	ASSERT_ERR(load_single_snprintf("%pB%"), "too many specifiers 6");
+	ASSERT_ERR(load_single_snprintf("%pi4%"), "too many specifiers 7");
+	ASSERT_ERR(load_single_snprintf("%pI4%"), "too many specifiers 8");
+	ASSERT_ERR(load_single_snprintf("%pi6%"), "too many specifiers 9");
+	ASSERT_ERR(load_single_snprintf("%pI6%"), "too many specifiers 10");
+	ASSERT_ERR(load_single_snprintf("%pks%"), "too many specifiers 11");
+	ASSERT_ERR(load_single_snprintf("%pus%"), "too many specifiers 12");
 }
 
 void test_snprintf(void)
-- 
2.43.0

Re: [PATCH bpf v2 1/2] bpf: fix %p% runtime check in bpf_bprintf_prepare

[Yonghong Song]

On 10/28/24 12:53 PM, Ilya Shchipletsov wrote:
> Fuzzing reports a warning in format_decode()
>
> Please remove unsupported %� in format string
> WARNING: CPU: 0 PID: 5091 at lib/vsprintf.c:2680 format_decode+0x1193/0x1bb0 lib/vsprintf.c:2680
> Modules linked in:
> CPU: 0 PID: 5091 Comm: syz-executor879 Not tainted 6.10.0-rc1-syzkaller-00021-ge0cce98fe279 #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 04/02/2024
> RIP: 0010:format_decode+0x1193/0x1bb0 lib/vsprintf.c:2680
> Call Trace:
>   <TASK>
>   bstr_printf+0x137/0x1210 lib/vsprintf.c:3253
>   ____bpf_trace_printk kernel/trace/bpf_trace.c:390 [inline]
>   bpf_trace_printk+0x1a1/0x230 kernel/trace/bpf_trace.c:375
>   bpf_prog_21da1b68f62e1237+0x36/0x41
>   bpf_dispatcher_nop_func include/linux/bpf.h:1243 [inline]
>   __bpf_prog_run include/linux/filter.h:691 [inline]
>   bpf_prog_run include/linux/filter.h:698 [inline]
>   bpf_test_run+0x40b/0x910 net/bpf/test_run.c:425
>   bpf_prog_test_run_skb+0xafa/0x13a0 net/bpf/test_run.c:1066
>   bpf_prog_test_run+0x33c/0x3b0 kernel/bpf/syscall.c:4291
>   __sys_bpf+0x48d/0x810 kernel/bpf/syscall.c:5705
>   __do_sys_bpf kernel/bpf/syscall.c:5794 [inline]
>   __se_sys_bpf kernel/bpf/syscall.c:5792 [inline]
>   __x64_sys_bpf+0x7c/0x90 kernel/bpf/syscall.c:5792
>   do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>   do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
>   entry_SYSCALL_64_after_hwframe+0x77/0x7f
>
> The problem occurs when trying to pass %p% at the end of format string,
> which would result in skipping last % and passing invalid format string
> down to format_decode() that would cause warning because of invalid
> character after %.
>
> Fix issue by advancing pointer only if next char is format modifier.
> If next char is null/space/punct, then just accept formatting as is,
> without advancing the pointer.
>
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
>
> Reported-by: syzbot+e2c932aec5c8a6e1d31c@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=e2c932aec5c8a6e1d31c
> Fixes: 48cac3f4a96d ("bpf: Implement formatted output helpers with bstr_printf")
> Co-developed-by: Nikita Marushkin <hfggklm@gmail.com>
> Signed-off-by: Nikita Marushkin <hfggklm@gmail.com>
> Signed-off-by: Ilya Shchipletsov <rabbelkin@mail.ru>

Acked-by: Yonghong Song <yonghong.song@linux.dev>

Re: [PATCH bpf v2 2/2] selftests/bpf: Add test cases for various pointer specifiers

[Yonghong Song]

On 10/28/24 12:53 PM, Ilya Shchipletsov wrote:
> Extend snprintf negative tests to cover pointer specifiers to prevent possible
> invalid handling of %p% from happening again.
>
>   ./test_progs -t snprintf
>   #302/1   snprintf/snprintf_positive:OK
>   #302/2   snprintf/snprintf_negative:OK
>   #302     snprintf:OK
>   #303     snprintf_btf:OK
>   Summary: 2/2 PASSED, 0 SKIPPED, 0 FAILED
>
> Co-developed-by: Nikita Marushkin <hfggklm@gmail.com>
> Signed-off-by: Nikita Marushkin <hfggklm@gmail.com>
> Signed-off-by: Ilya Shchipletsov <rabbelkin@mail.ru>

Acked-by: Yonghong Song <yonghong.song@linux.dev>

Re: [PATCH bpf v2 1/2] bpf: fix %p% runtime check in bpf_bprintf_prepare

[Florent Revest]

Acked-by: Florent Revest <revest@chromium.org>

On Tue, Oct 29, 2024 at 7:18 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>
>
> On 10/28/24 12:53 PM, Ilya Shchipletsov wrote:
> > Fuzzing reports a warning in format_decode()
> >
> > Please remove unsupported %� in format string
> > WARNING: CPU: 0 PID: 5091 at lib/vsprintf.c:2680 format_decode+0x1193/0x1bb0 lib/vsprintf.c:2680
> > Modules linked in:
> > CPU: 0 PID: 5091 Comm: syz-executor879 Not tainted 6.10.0-rc1-syzkaller-00021-ge0cce98fe279 #0
> > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 04/02/2024
> > RIP: 0010:format_decode+0x1193/0x1bb0 lib/vsprintf.c:2680
> > Call Trace:
> >   <TASK>
> >   bstr_printf+0x137/0x1210 lib/vsprintf.c:3253
> >   ____bpf_trace_printk kernel/trace/bpf_trace.c:390 [inline]
> >   bpf_trace_printk+0x1a1/0x230 kernel/trace/bpf_trace.c:375
> >   bpf_prog_21da1b68f62e1237+0x36/0x41
> >   bpf_dispatcher_nop_func include/linux/bpf.h:1243 [inline]
> >   __bpf_prog_run include/linux/filter.h:691 [inline]
> >   bpf_prog_run include/linux/filter.h:698 [inline]
> >   bpf_test_run+0x40b/0x910 net/bpf/test_run.c:425
> >   bpf_prog_test_run_skb+0xafa/0x13a0 net/bpf/test_run.c:1066
> >   bpf_prog_test_run+0x33c/0x3b0 kernel/bpf/syscall.c:4291
> >   __sys_bpf+0x48d/0x810 kernel/bpf/syscall.c:5705
> >   __do_sys_bpf kernel/bpf/syscall.c:5794 [inline]
> >   __se_sys_bpf kernel/bpf/syscall.c:5792 [inline]
> >   __x64_sys_bpf+0x7c/0x90 kernel/bpf/syscall.c:5792
> >   do_syscall_x64 arch/x86/entry/common.c:52 [inline]
> >   do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
> >   entry_SYSCALL_64_after_hwframe+0x77/0x7f
> >
> > The problem occurs when trying to pass %p% at the end of format string,
> > which would result in skipping last % and passing invalid format string
> > down to format_decode() that would cause warning because of invalid
> > character after %.
> >
> > Fix issue by advancing pointer only if next char is format modifier.
> > If next char is null/space/punct, then just accept formatting as is,
> > without advancing the pointer.
> >
> > Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
> >
> > Reported-by: syzbot+e2c932aec5c8a6e1d31c@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=e2c932aec5c8a6e1d31c
> > Fixes: 48cac3f4a96d ("bpf: Implement formatted output helpers with bstr_printf")
> > Co-developed-by: Nikita Marushkin <hfggklm@gmail.com>
> > Signed-off-by: Nikita Marushkin <hfggklm@gmail.com>
> > Signed-off-by: Ilya Shchipletsov <rabbelkin@mail.ru>
>
> Acked-by: Yonghong Song <yonghong.song@linux.dev>
>

Re: [PATCH bpf v2 2/2] selftests/bpf: Add test cases for various pointer specifiers

[Florent Revest]

Thank you!

Acked-by: Florent Revest <revest@chromium.org>

On Tue, Oct 29, 2024 at 7:19 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>
>
> On 10/28/24 12:53 PM, Ilya Shchipletsov wrote:
> > Extend snprintf negative tests to cover pointer specifiers to prevent possible
> > invalid handling of %p% from happening again.
> >
> >   ./test_progs -t snprintf
> >   #302/1   snprintf/snprintf_positive:OK
> >   #302/2   snprintf/snprintf_negative:OK
> >   #302     snprintf:OK
> >   #303     snprintf_btf:OK
> >   Summary: 2/2 PASSED, 0 SKIPPED, 0 FAILED
> >
> > Co-developed-by: Nikita Marushkin <hfggklm@gmail.com>
> > Signed-off-by: Nikita Marushkin <hfggklm@gmail.com>
> > Signed-off-by: Ilya Shchipletsov <rabbelkin@mail.ru>
>
> Acked-by: Yonghong Song <yonghong.song@linux.dev>
>

Re: [PATCH bpf v2 1/2] bpf: fix %p% runtime check in bpf_bprintf_prepare

[Fedor Pchelkin]

On Thu, 31. Oct 17:17, Florent Revest wrote:
> Acked-by: Florent Revest <revest@chromium.org>
> 
> On Tue, Oct 29, 2024 at 7:18 AM Yonghong Song <yonghong.song@linux.dev> wrote:
> > On 10/28/24 12:53 PM, Ilya Shchipletsov wrote:
> > > Fuzzing reports a warning in format_decode()
> > >
> > > Please remove unsupported %� in format string
> > > WARNING: CPU: 0 PID: 5091 at lib/vsprintf.c:2680 format_decode+0x1193/0x1bb0 lib/vsprintf.c:2680
> > > Modules linked in:
> > > CPU: 0 PID: 5091 Comm: syz-executor879 Not tainted 6.10.0-rc1-syzkaller-00021-ge0cce98fe279 #0
> > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 04/02/2024
> > > RIP: 0010:format_decode+0x1193/0x1bb0 lib/vsprintf.c:2680
> > > Call Trace:
> > >   <TASK>
> > >   bstr_printf+0x137/0x1210 lib/vsprintf.c:3253
> > >   ____bpf_trace_printk kernel/trace/bpf_trace.c:390 [inline]
> > >   bpf_trace_printk+0x1a1/0x230 kernel/trace/bpf_trace.c:375
> > >   bpf_prog_21da1b68f62e1237+0x36/0x41
> > >   bpf_dispatcher_nop_func include/linux/bpf.h:1243 [inline]
> > >   __bpf_prog_run include/linux/filter.h:691 [inline]
> > >   bpf_prog_run include/linux/filter.h:698 [inline]
> > >   bpf_test_run+0x40b/0x910 net/bpf/test_run.c:425
> > >   bpf_prog_test_run_skb+0xafa/0x13a0 net/bpf/test_run.c:1066
> > >   bpf_prog_test_run+0x33c/0x3b0 kernel/bpf/syscall.c:4291
> > >   __sys_bpf+0x48d/0x810 kernel/bpf/syscall.c:5705
> > >   __do_sys_bpf kernel/bpf/syscall.c:5794 [inline]
> > >   __se_sys_bpf kernel/bpf/syscall.c:5792 [inline]
> > >   __x64_sys_bpf+0x7c/0x90 kernel/bpf/syscall.c:5792
> > >   do_syscall_x64 arch/x86/entry/common.c:52 [inline]
> > >   do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
> > >   entry_SYSCALL_64_after_hwframe+0x77/0x7f
> > >
> > > The problem occurs when trying to pass %p% at the end of format string,
> > > which would result in skipping last % and passing invalid format string
> > > down to format_decode() that would cause warning because of invalid
> > > character after %.
> > >
> > > Fix issue by advancing pointer only if next char is format modifier.
> > > If next char is null/space/punct, then just accept formatting as is,
> > > without advancing the pointer.
> > >
> > > Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
> > >
> > > Reported-by: syzbot+e2c932aec5c8a6e1d31c@syzkaller.appspotmail.com
> > > Closes: https://syzkaller.appspot.com/bug?extid=e2c932aec5c8a6e1d31c
> > > Fixes: 48cac3f4a96d ("bpf: Implement formatted output helpers with bstr_printf")
> > > Co-developed-by: Nikita Marushkin <hfggklm@gmail.com>
> > > Signed-off-by: Nikita Marushkin <hfggklm@gmail.com>
> > > Signed-off-by: Ilya Shchipletsov <rabbelkin@mail.ru>
> >
> > Acked-by: Yonghong Song <yonghong.song@linux.dev>
> >

The patches of the series are marked as "Changes Requested" in Patchwork.
They've been acked twice. And there've been no additional comment posted
on mailing lists explaining the "Changes Required" status.

Is there anything to improve in the series? Or it can be applied as is?

--
Thanks,
Fedor