linux-kernel - Re: [PATCH v1] rtla: Fix implicit NULL dereference

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <16990bcc-c328-457b-a789-0f318d8eaf3f@stanley.mountain>
Date: Fri, 10 Jan 2025 08:38:35 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Costa Shulyupin <costa.shul@...hat.com>
Cc: Steven Rostedt <rostedt@...dmis.org>,
	Daniel Bristot de Oliveira <bristot@...nel.org>,
	John Kacur <jkacur@...hat.com>,
	"Luis Claudio R. Goncalves" <lgoncalv@...hat.com>,
	Eder Zulian <ezulian@...hat.com>, Tomas Glozar <tglozar@...hat.com>,
	Gabriele Monaco <gmonaco@...hat.com>,
	linux-trace-kernel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] rtla: Fix implicit NULL dereference

The subject is bad because it says "Fix" when this is a clean up
and it says "NULL dereference" when there isn't any NULL dereference.

On Thu, Jan 09, 2025 at 11:13:26PM +0200, Costa Shulyupin wrote:
> The `record` variable is NULL when tracing is not requested:
> 
> struct osnoise_tool *record = NULL;
> 
> if (params->trace_output) {
>         record = osnoise_init_trace_tool("osnoise");
> ....
> 
> Value of `&record->trace` in this case is NULL just because
> the `trace` member is the first member `struct osnoise_tool` with offset 0.
> `&record->trace` just returns the offset.
> 
> Explicit dereference `record->trace' would cause segmentation fault.
> 
> Add explicit check for zero `record`.
> 

This commit message is very confusing.  I would normally not send a
patch like this, but if I did send it the commit message would say
something like:

  The "record" pointer can be NULL in this code.  When we're calling
  trace_is_off(&tool->trace, &record->trace) and "record" is NULL then
  it kind of looks like a NULL dereference.  It turns out that it's
  fine when you look at it more closely, but at first glance it looks
  sketchy.  Add an explicit NULL check to make the code more clear.

Tracing code is generally fast path code so maybe we don't want to add
a NULL check?  If we were really bothered by the existing code then a
better fix would be to add an inline function to do it.

regards,
dan carpenter