* [PATCH] trace: fix null pointer dereference in tracing_err_log_open() [not found] <CGME20230703155237eucms1p4dfb6a19caa14c79eb6c823d127b39024@eucms1p4> @ 2023-07-03 15:52 ` Mateusz Stachyra 2023-07-03 16:18 ` Steven Rostedt 0 siblings, 1 reply; 3+ messages in thread From: Mateusz Stachyra @ 2023-07-03 15:52 UTC (permalink / raw) To: linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org Cc: rostedt@goodmis.org, mhiramat@kernel.org From d6ef949d29b884dd77fe5e628dc71318de08868c Mon Sep 17 00:00:00 2001 From: Mateusz Stachyra <m.stachyra@samsung.com> Date: Mon, 3 Jul 2023 17:48:40 +0200 Subject: [PATCH] trace: fix null pointer dereference in tracing_err_log_open() Fix an issue in function 'tracing_err_log_open'. The function doesn't call 'seq_open' if file is opened only with write permissions, which results in 'file->private_data' being left at null. If we then use 'lseek' on that opened file, 'seq_lseek' dereferences 'file->private_data' in 'mutex_lock(&m->lock)', resulting in a Kernel panic. Writing to this node requires root privilages, therefore this bug has very little security impact. Tracefs node: /sys/kernel/tracing/error_log Example Kernel panic: Unable to handle kernel NULL pointer dereference at virtual address 0000000000000038 Call trace: mutex_lock+0x30/0x110 seq_lseek+0x34/0xb8 __arm64_sys_lseek+0x6c/0xb8 invoke_syscall+0x58/0x13c el0_svc_common+0xc4/0x10c do_el0_svc+0x24/0x98 el0_svc+0x24/0x88 el0t_64_sync_handler+0x84/0xe4 el0t_64_sync+0x1b4/0x1b8 Code: d503201f aa0803e0 aa1f03e1 aa0103e9 (c8e97d02) ---[ end trace 561d1b49c12cf8a5 ]--- Kernel panic - not syncing: Oops: Fatal exception Signed-off-by: Mateusz Stachyra <m.stachyra@samsung.com> --- kernel/trace/trace.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index 5d2c5678b..bfa8e2d01 100644 --- a/kernel/trace/trace.c +++ b/kernel/trace/trace.c @@ -8097,8 +8097,16 @@ static int tracing_err_log_open(struct inode *inode, struct file *file) return ret; /* If this file was opened for write, then erase contents */ - if ((file->f_mode & FMODE_WRITE) && (file->f_flags & O_TRUNC)) + if ((file->f_mode & FMODE_WRITE) && (file->f_flags & O_TRUNC)) { clear_tracing_err_log(tr); + ret = seq_open(file, &tracing_err_log_seq_ops); + if (!ret) { + struct seq_file *m = file->private_data; + m->private = tr; + } else { + trace_array_put(tr); + } + } if (file->f_mode & FMODE_READ) { ret = seq_open(file, &tracing_err_log_seq_ops); base-commit: 1ef6663a587ba3e57dc5065a477db1c64481eedd -- 2.25.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] trace: fix null pointer dereference in tracing_err_log_open() 2023-07-03 15:52 ` [PATCH] trace: fix null pointer dereference in tracing_err_log_open() Mateusz Stachyra @ 2023-07-03 16:18 ` Steven Rostedt 2023-07-04 10:34 ` Mateusz Stachyra 0 siblings, 1 reply; 3+ messages in thread From: Steven Rostedt @ 2023-07-03 16:18 UTC (permalink / raw) To: Mateusz Stachyra Cc: linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org, mhiramat@kernel.org On Mon, 03 Jul 2023 17:52:37 +0200 Mateusz Stachyra <m.stachyra@samsung.com> wrote: > >From d6ef949d29b884dd77fe5e628dc71318de08868c Mon Sep 17 00:00:00 2001 > From: Mateusz Stachyra <m.stachyra@samsung.com> > Date: Mon, 3 Jul 2023 17:48:40 +0200 > Subject: [PATCH] trace: fix null pointer dereference in tracing_err_log_open() > > Fix an issue in function 'tracing_err_log_open'. > The function doesn't call 'seq_open' if file is opened only with > write permissions, which results in 'file->private_data' being left at null. > If we then use 'lseek' on that opened file, 'seq_lseek' dereferences > 'file->private_data' in 'mutex_lock(&m->lock)', resulting in a Kernel panic. > Writing to this node requires root privilages, therefore this bug > has very little security impact. > > Tracefs node: /sys/kernel/tracing/error_log > Nice catch, but I recommend a different solution. > Example Kernel panic: > > Unable to handle kernel NULL pointer dereference at virtual address 0000000000000038 > Call trace: > mutex_lock+0x30/0x110 > seq_lseek+0x34/0xb8 > __arm64_sys_lseek+0x6c/0xb8 > invoke_syscall+0x58/0x13c > el0_svc_common+0xc4/0x10c > do_el0_svc+0x24/0x98 > el0_svc+0x24/0x88 > el0t_64_sync_handler+0x84/0xe4 > el0t_64_sync+0x1b4/0x1b8 > Code: d503201f aa0803e0 aa1f03e1 aa0103e9 (c8e97d02) > ---[ end trace 561d1b49c12cf8a5 ]--- > Kernel panic - not syncing: Oops: Fatal exception > > Signed-off-by: Mateusz Stachyra <m.stachyra@samsung.com> > --- > kernel/trace/trace.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > index 5d2c5678b..bfa8e2d01 100644 > --- a/kernel/trace/trace.c > +++ b/kernel/trace/trace.c > @@ -8097,8 +8097,16 @@ static int tracing_err_log_open(struct inode *inode, struct file *file) > return ret; > > /* If this file was opened for write, then erase contents */ > - if ((file->f_mode & FMODE_WRITE) && (file->f_flags & O_TRUNC)) > + if ((file->f_mode & FMODE_WRITE) && (file->f_flags & O_TRUNC)) { > clear_tracing_err_log(tr); > + ret = seq_open(file, &tracing_err_log_seq_ops); > + if (!ret) { > + struct seq_file *m = file->private_data; > + m->private = tr; > + } else { > + trace_array_put(tr); > + } > + } > > if (file->f_mode & FMODE_READ) { > ret = seq_open(file, &tracing_err_log_seq_ops); > > base-commit: 1ef6663a587ba3e57dc5065a477db1c64481eedd I believe this can be better fixed by: diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index 64a4dde073ef..999b7c73e324 100644 --- a/kernel/trace/trace.c +++ b/kernel/trace/trace.c @@ -8135,7 +8135,7 @@ static const struct file_operations tracing_err_log_fops = { .open = tracing_err_log_open, .write = tracing_err_log_write, .read = seq_read, - .llseek = seq_lseek, + .llseek = tracing_lseek, .release = tracing_err_log_release, }; as that tracing_lseek() is for this exact scenario. Care to send a v2? Thanks, -- Steve ^ permalink raw reply related [flat|nested] 3+ messages in thread
* RE: Re: [PATCH] trace: fix null pointer dereference in tracing_err_log_open() 2023-07-03 16:18 ` Steven Rostedt @ 2023-07-04 10:34 ` Mateusz Stachyra 0 siblings, 0 replies; 3+ messages in thread From: Mateusz Stachyra @ 2023-07-04 10:34 UTC (permalink / raw) To: Steven Rostedt Cc: linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org, mhiramat@kernel.org >> >From d6ef949d29b884dd77fe5e628dc71318de08868c Mon Sep 17 00:00:00 2001 >> From: Mateusz Stachyra <m.stachyra@samsung.com> >> Date: Mon, 3 Jul 2023 17:48:40 +0200 >> Subject: [PATCH] trace: fix null pointer dereference in tracing_err_log_open() >> >> Fix an issue in function 'tracing_err_log_open'. >> The function doesn't call 'seq_open' if file is opened only with >> write permissions, which results in 'file->private_data' being left at null. >> If we then use 'lseek' on that opened file, 'seq_lseek' dereferences >> 'file->private_data' in 'mutex_lock(&m->lock)', resulting in a Kernel panic. >> Writing to this node requires root privilages, therefore this bug >> has very little security impact. >> >> Tracefs node: /sys/kernel/tracing/error_log >> > >Nice catch, but I recommend a different solution. > > >> Example Kernel panic: >> >> Unable to handle kernel NULL pointer dereference at virtual address 0000000000000038 >> Call trace: >> mutex_lock+0x30/0x110 >> seq_lseek+0x34/0xb8 >> __arm64_sys_lseek+0x6c/0xb8 >> invoke_syscall+0x58/0x13c >> el0_svc_common+0xc4/0x10c >> do_el0_svc+0x24/0x98 >> el0_svc+0x24/0x88 >> el0t_64_sync_handler+0x84/0xe4 >> el0t_64_sync+0x1b4/0x1b8 >> Code: d503201f aa0803e0 aa1f03e1 aa0103e9 (c8e97d02) >> ---[ end trace 561d1b49c12cf8a5 ]--- >> Kernel panic - not syncing: Oops: Fatal exception >> >> Signed-off-by: Mateusz Stachyra <m.stachyra@samsung.com> >> --- >> kernel/trace/trace.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c >> index 5d2c5678b..bfa8e2d01 100644 >> --- a/kernel/trace/trace.c >> +++ b/kernel/trace/trace.c >> @@ -8097,8 +8097,16 @@ static int tracing_err_log_open(struct inode *inode, struct file *file) >> return ret; >> >> /* If this file was opened for write, then erase contents */ >> - if ((file->f_mode & FMODE_WRITE) && (file->f_flags & O_TRUNC)) >> + if ((file->f_mode & FMODE_WRITE) && (file->f_flags & O_TRUNC)) { >> clear_tracing_err_log(tr); >> + ret = seq_open(file, &tracing_err_log_seq_ops); >> + if (!ret) { >> + struct seq_file *m = file->private_data; >> + m->private = tr; >> + } else { >> + trace_array_put(tr); >> + } >> + } >> >> if (file->f_mode & FMODE_READ) { >> ret = seq_open(file, &tracing_err_log_seq_ops); >> >> base-commit: 1ef6663a587ba3e57dc5065a477db1c64481eedd > >I believe this can be better fixed by: > >diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c >index 64a4dde073ef..999b7c73e324 100644 >--- a/kernel/trace/trace.c >+++ b/kernel/trace/trace.c >@@ -8135,7 +8135,7 @@ static const struct file_operations tracing_err_log_fops = { > .open = tracing_err_log_open, > .write = tracing_err_log_write, > .read = seq_read, >- .llseek = seq_lseek, >+ .llseek = tracing_lseek, > .release = tracing_err_log_release, > }; > >as that tracing_lseek() is for this exact scenario. > >Care to send a v2? > >Thanks, > >-- Steve Thanks for reply and for the suggestion. I've submitted v2 at: https://lore.kernel.org/all/20230704102706eucms1p30d7ecdcc287f46ad67679fc8491b2e0f@eucms1p3/ -- Mateusz ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-07-04 10:34 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CGME20230703155237eucms1p4dfb6a19caa14c79eb6c823d127b39024@eucms1p4>
2023-07-03 15:52 ` [PATCH] trace: fix null pointer dereference in tracing_err_log_open() Mateusz Stachyra
2023-07-03 16:18 ` Steven Rostedt
2023-07-04 10:34 ` Mateusz Stachyra
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox