Messages in this thread | | | Subject | RE: Re: [PATCH] trace: fix null pointer dereference in tracing_err_log_open() | From | Mateusz Stachyra <> | Date | Tue, 04 Jul 2023 12:34:31 +0200 |
| |
>> >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
| |