Message ID | 20250313103719.1191073-1-wutengda@huaweicloud.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | tracing: Correct the refcount for hist/hist_debug file if single_open() fails | expand |
On Thu, 13 Mar 2025 10:37:19 +0000 Tengda Wu <wutengda@huaweicloud.com> wrote: > The function event_{hist,hist_debug}_open() maintains the refcount of > 'file->tr' and 'file' through tracing_open_file_tr(), but it does not > roll back these counts when the subsequent single_open() call fails, > leading to a refcount leak. > > A very obvious case is that if the hist/hist_debug file belongs to a > certain instance, the failure of single_open() will prevent the deletion > of that instance, as it relies on the condition 'tr->ref == 1' within > __remove_instance(). > > Fix this by calling tracing_release_file_tr() to correct the refcount > when single_open() fails. > > Fixes: 1cc111b9cddc ("tracing: Fix uaf issue when open the hist or hist_debug file") > Signed-off-by: Tengda Wu <wutengda@huaweicloud.com> > Cc: stable@vger.kernel.org # v6.7+ > --- > kernel/trace/trace_events_hist.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c > index ad7419e24055..900b06fa8505 100644 > --- a/kernel/trace/trace_events_hist.c > +++ b/kernel/trace/trace_events_hist.c > @@ -5702,8 +5702,10 @@ static int event_hist_open(struct inode *inode, struct file *file) > /* Clear private_data to avoid warning in single_open() */ > file->private_data = NULL; > ret = single_open(file, hist_show, hist_file); > - if (ret) > + if (ret) { > kfree(hist_file); > + tracing_release_file_tr(inode, file); > + } Hmm, this function has a couple more error path exits after taking the ref count. If this is to be fixed, let's fix it completely. Each of those error paths need to call tracing_release_file_tr(). -- Steve > > return ret; > } > @@ -5979,7 +5981,10 @@ static int event_hist_debug_open(struct inode *inode, struct file *file) > > /* Clear private_data to avoid warning in single_open() */ > file->private_data = NULL; > - return single_open(file, hist_debug_show, file); > + ret = single_open(file, hist_debug_show, file); > + if (ret) > + tracing_release_file_tr(inode, file); > + return ret; > } > > const struct file_operations event_hist_debug_fops = {
Hi Steven, On 2025/3/13 22:44, Steven Rostedt wrote: > On Thu, 13 Mar 2025 10:37:19 +0000 > Tengda Wu <wutengda@huaweicloud.com> wrote: > >> The function event_{hist,hist_debug}_open() maintains the refcount of >> 'file->tr' and 'file' through tracing_open_file_tr(), but it does not >> roll back these counts when the subsequent single_open() call fails, >> leading to a refcount leak. >> >> A very obvious case is that if the hist/hist_debug file belongs to a >> certain instance, the failure of single_open() will prevent the deletion >> of that instance, as it relies on the condition 'tr->ref == 1' within >> __remove_instance(). >> >> Fix this by calling tracing_release_file_tr() to correct the refcount >> when single_open() fails. >> >> Fixes: 1cc111b9cddc ("tracing: Fix uaf issue when open the hist or hist_debug file") >> Signed-off-by: Tengda Wu <wutengda@huaweicloud.com> >> Cc: stable@vger.kernel.org # v6.7+ >> --- >> kernel/trace/trace_events_hist.c | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c >> index ad7419e24055..900b06fa8505 100644 >> --- a/kernel/trace/trace_events_hist.c >> +++ b/kernel/trace/trace_events_hist.c >> @@ -5702,8 +5702,10 @@ static int event_hist_open(struct inode *inode, struct file *file) >> /* Clear private_data to avoid warning in single_open() */ >> file->private_data = NULL; >> ret = single_open(file, hist_show, hist_file); >> - if (ret) >> + if (ret) { >> kfree(hist_file); >> + tracing_release_file_tr(inode, file); >> + } > > Hmm, this function has a couple more error path exits after taking the > ref count. If this is to be fixed, let's fix it completely. Each of > those error paths need to call tracing_release_file_tr(). > > -- Steve Oops, I got it. The v2 is coming soon. Thanks a lot. -- Tengda > > >> >> return ret; >> } >> @@ -5979,7 +5981,10 @@ static int event_hist_debug_open(struct inode *inode, struct file *file) >> >> /* Clear private_data to avoid warning in single_open() */ >> file->private_data = NULL; >> - return single_open(file, hist_debug_show, file); >> + ret = single_open(file, hist_debug_show, file); >> + if (ret) >> + tracing_release_file_tr(inode, file); >> + return ret; >> } >> >> const struct file_operations event_hist_debug_fops = {
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c index ad7419e24055..900b06fa8505 100644 --- a/kernel/trace/trace_events_hist.c +++ b/kernel/trace/trace_events_hist.c @@ -5702,8 +5702,10 @@ static int event_hist_open(struct inode *inode, struct file *file) /* Clear private_data to avoid warning in single_open() */ file->private_data = NULL; ret = single_open(file, hist_show, hist_file); - if (ret) + if (ret) { kfree(hist_file); + tracing_release_file_tr(inode, file); + } return ret; } @@ -5979,7 +5981,10 @@ static int event_hist_debug_open(struct inode *inode, struct file *file) /* Clear private_data to avoid warning in single_open() */ file->private_data = NULL; - return single_open(file, hist_debug_show, file); + ret = single_open(file, hist_debug_show, file); + if (ret) + tracing_release_file_tr(inode, file); + return ret; } const struct file_operations event_hist_debug_fops = {
The function event_{hist,hist_debug}_open() maintains the refcount of 'file->tr' and 'file' through tracing_open_file_tr(), but it does not roll back these counts when the subsequent single_open() call fails, leading to a refcount leak. A very obvious case is that if the hist/hist_debug file belongs to a certain instance, the failure of single_open() will prevent the deletion of that instance, as it relies on the condition 'tr->ref == 1' within __remove_instance(). Fix this by calling tracing_release_file_tr() to correct the refcount when single_open() fails. Fixes: 1cc111b9cddc ("tracing: Fix uaf issue when open the hist or hist_debug file") Signed-off-by: Tengda Wu <wutengda@huaweicloud.com> Cc: stable@vger.kernel.org # v6.7+ --- kernel/trace/trace_events_hist.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)