linux-kernel - Re: [PATCH] [PATCH] cxl: trace: Fix macro safety in CXL_EVT_TP_fast

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] [day] [month] [year] [list]

Message-ID: <20250421151239.7981f1fa@gandalf.local.home>
Date: Mon, 21 Apr 2025 15:12:39 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: "shaikh.kamal" <shaikhkamal2012@...il.com>
Cc: dan.j.williams@...el.com, Davidlohr Bueso <dave@...olabs.net>, Jonathan
 Cameron <jonathan.cameron@...wei.com>, Dave Jiang <dave.jiang@...el.com>,
 Alison Schofield <alison.schofield@...el.com>, Vishal Verma
 <vishal.l.verma@...el.com>, Ira Weiny <ira.weiny@...el.com>, Shiju Jose
 <shiju.jose@...wei.com>, "Fabio M. De Francesco"
 <fabio.m.de.francesco@...ux.intel.com>, Smita Koralahalli
 <Smita.KoralahalliChannabasappa@....com>, linux-cxl@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] [PATCH] cxl: trace: Fix macro safety in
 CXL_EVT_TP_fast_assign

On Tue, 22 Apr 2025 00:15:17 +0530
"shaikh.kamal" <shaikhkamal2012@...il.com> wrote:

> Fix checkpatch.pl detected error

First, checkpatch.pl should never be used on existing code unless it's
yours. As the name suggests, it's for checking patches. It's not to check
what's already been accepted. Please do not submit patches against accepted
code because of what checkpatch.pl reports.

If you run it on code and it reports something that you find is a real bug,
then sure, fix it. But don't submit patches on code you do not understand
just because checkpatch.pl says there's an issue with it.

> The CXL_EVT_TP_fast_assign macro assigns multiple fields, but does not
> wrap the body in a `do { ... } while (0)` block. This can lead to
> unexpected behavior when used in conditional branches.

Next, this is not a normal macro. It's a trace event macro and
checkpatch.pl fails miserably on pretty much all tracing macros.

> 
> Add checks to ensure cxlmd is valid before accessing its fields.

If it is invalid, and we do what your patch suggests and just not write
anything, the event it was writing into has already been created. If we
exit out of this macro, then the event will simply contain garbage, but is
already on its way to user space. That means the output to user space will
be garbage. That's a bug. In other words, if cxlmd is NULL, it had better
not be calling this macro in the fist place!

-- Steve