[PATCH] tracing: fix oob write in trace_seq_to_buffer()

[PATCH] tracing: fix oob write in trace_seq_to_buffer()

[Jeongjun Park]

syzbot reported this bug:
==================================================================
BUG: KASAN: slab-out-of-bounds in trace_seq_to_buffer kernel/trace/trace.c:1830 [inline]
BUG: KASAN: slab-out-of-bounds in tracing_splice_read_pipe+0x6be/0xdd0 kernel/trace/trace.c:6822
Write of size 4507 at addr ffff888032b6b000 by task syz.2.320/7260

CPU: 1 UID: 0 PID: 7260 Comm: syz.2.320 Not tainted 6.15.0-rc1-syzkaller-00301-g3bde70a2c827 #0 PREEMPT(full) 
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 02/12/2025
Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:94 [inline]
 dump_stack_lvl+0x116/0x1f0 lib/dump_stack.c:120
 print_address_description mm/kasan/report.c:408 [inline]
 print_report+0xc3/0x670 mm/kasan/report.c:521
 kasan_report+0xe0/0x110 mm/kasan/report.c:634
 check_region_inline mm/kasan/generic.c:183 [inline]
 kasan_check_range+0xef/0x1a0 mm/kasan/generic.c:189
 __asan_memcpy+0x3c/0x60 mm/kasan/shadow.c:106
 trace_seq_to_buffer kernel/trace/trace.c:1830 [inline]
 tracing_splice_read_pipe+0x6be/0xdd0 kernel/trace/trace.c:6822
 ....
==================================================================

It has been reported that trace_seq_to_buffer() attempts to copy more than
PAGE_SIZE data into buf, so we need to add code to check the size of the 
cnt value to prevent this.

Reported-by: syzbot+c8cd2d2c412b868263fb@syzkaller.appspotmail.com
Fixes: 3c56819b14b0 ("tracing: splice support for tracing_pipe")
Signed-off-by: Jeongjun Park <aha310510@gmail.com>
---
 kernel/trace/trace.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 8ddf6b17215c..8ba6ea38411d 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -1827,6 +1827,8 @@ static ssize_t trace_seq_to_buffer(struct trace_seq *s, void *buf, size_t cnt)
 	len = trace_seq_used(s) - s->readpos;
 	if (cnt > len)
 		cnt = len;
+	if (cnt > PAGE_SIZE)
+		return -EINVAL;
 	memcpy(buf, s->buffer + s->readpos, cnt);
 
 	s->readpos += cnt;
--

Re: [PATCH] tracing: fix oob write in trace_seq_to_buffer()

[Steven Rostedt]

On Mon, 21 Apr 2025 22:49:36 +0900
Jeongjun Park <aha310510@gmail.com> wrote:

> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 8ddf6b17215c..8ba6ea38411d 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -1827,6 +1827,8 @@ static ssize_t trace_seq_to_buffer(struct trace_seq *s, void *buf, size_t cnt)
>  	len = trace_seq_used(s) - s->readpos;
>  	if (cnt > len)
>  		cnt = len;
> +	if (cnt > PAGE_SIZE)
> +		return -EINVAL;

You fixed the wrong location. The caller should know how much size the
buffer is, and that's passed in by cnt.

>  	memcpy(buf, s->buffer + s->readpos, cnt);
>  
>  	s->readpos += cnt;

The correct fix would be:

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index b6e40e8791fa..c23b5ab27314 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -6729,7 +6864,8 @@ static ssize_t tracing_splice_read_pipe(struct file *filp,
 		/* Copy the data into the page, so we can start over. */
 		ret = trace_seq_to_buffer(&iter->seq,
 					  page_address(spd.pages[i]),
-					  trace_seq_used(&iter->seq));
+					  min(trace_seq_used(&iter->seq),
+					      PAGE_SIZE));
 		if (ret < 0) {
 			__free_page(spd.pages[i]);
 			break;

Especially since the trace_seq_to_buffer() code should be moved out of this
file and should have no idea how big the buffer passed in is.

-- Steve

Re: [PATCH] tracing: fix oob write in trace_seq_to_buffer()

[Jeongjun Park]

Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Mon, 21 Apr 2025 22:49:36 +0900
> Jeongjun Park <aha310510@gmail.com> wrote:
>
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index 8ddf6b17215c..8ba6ea38411d 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -1827,6 +1827,8 @@ static ssize_t trace_seq_to_buffer(struct trace_seq *s, void *buf, size_t cnt)
> >       len = trace_seq_used(s) - s->readpos;
> >       if (cnt > len)
> >               cnt = len;
> > +     if (cnt > PAGE_SIZE)
> > +             return -EINVAL;
>
> You fixed the wrong location. The caller should know how much size the
> buffer is, and that's passed in by cnt.
>
> >       memcpy(buf, s->buffer + s->readpos, cnt);
> >
> >       s->readpos += cnt;
>
> The correct fix would be:
>
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index b6e40e8791fa..c23b5ab27314 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -6729,7 +6864,8 @@ static ssize_t tracing_splice_read_pipe(struct file *filp,
>                 /* Copy the data into the page, so we can start over. */
>                 ret = trace_seq_to_buffer(&iter->seq,
>                                           page_address(spd.pages[i]),
> -                                         trace_seq_used(&iter->seq));
> +                                         min(trace_seq_used(&iter->seq),
> +                                             PAGE_SIZE));
>                 if (ret < 0) {
>                         __free_page(spd.pages[i]);
>                         break;
>
> Especially since the trace_seq_to_buffer() code should be moved out of this
> file and should have no idea how big the buffer passed in is.

Thanks for your advice! I'll fix it right away and send you a v2 patch.

Regards,

Jeongjun Park

>
> -- Steve