linux-kernel - Re: [PATCH] scsi: fix uninitialized pointers with free attr

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: <aRwPcgDXSE9s4jKS@stanley.mountain>
Date: Tue, 18 Nov 2025 09:17:22 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: James Bottomley <James.Bottomley@...senpartnership.com>
Cc: Ally Heev <allyheev@...il.com>,
	"Martin K. Petersen" <martin.petersen@...cle.com>,
	linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] scsi: fix uninitialized pointers with free attr

On Thu, Nov 06, 2025 at 11:06:29AM -0500, James Bottomley wrote:
> On Thu, 2025-11-06 at 17:46 +0300, Dan Carpenter wrote:
> > On Wed, Nov 05, 2025 at 10:32:19AM -0500, James Bottomley wrote:
> > > > > > diff --git a/drivers/scsi/scsi_debug.c
> > > > > > b/drivers/scsi/scsi_debug.c
> > > > > > index
> > > > > > b2ab97be5db3d43d5a5647968623b8db72448379..89b36d65926bdd15c0a
> > > > > > e93a
> > > > > > 6bd2
> > > > > > ea968e25c0e74 100644
> > > > > > --- a/drivers/scsi/scsi_debug.c
> > > > > > +++ b/drivers/scsi/scsi_debug.c
> > > > > > @@ -2961,11 +2961,11 @@ static int resp_mode_sense(struct
> > > > > > scsi_cmnd
> > > > > > *scp,
> > > > > >  	int target_dev_id;
> > > > > >  	int target = scp->device->id;
> > > > > >  	unsigned char *ap;
> > > > > > -	unsigned char *arr __free(kfree);
> > > > > >  	unsigned char *cmd = scp->cmnd;
> > > > > >  	bool dbd, llbaa, msense_6, is_disk, is_zbc, is_tape;
> > > > > >  
> > > > > > -	arr = kzalloc(SDEBUG_MAX_MSENSE_SZ, GFP_ATOMIC);
> > > > > > +	unsigned char *arr __free(kfree) =
> > > > > > kzalloc(SDEBUG_MAX_MSENSE_SZ, GFP_ATOMIC);
> > > > > > +
> > > > > 
> > > > > Moving variable assignments inside code makes it way harder to
> > > > > read. Given that compilers will eventually detect if we do a
> > > > > return before initialization, can't you have smatch do the same
> > > > > rather than trying to force something like this?
> > > > 
> > > > This isn't a Smatch thing, it's a change to checkpatch.
> > > > 
> > > > (Smatch does work as you describe).
> > > 
> > > So why are we bothering with something like this in checkpatch if
> > > we can detect the true problem condition and we expect compilers to
> > > catch up?  Encouraging people to write code like the above isn't in
> > > anyone's best interest.
> > 
> > Initializing __free variables has been considered best practice for a
> > long time.  Reviewers often will complain even if it doesn't cause a
> > bug.
> 
> Well, not responsible for the daft ideas other people have.
> 
> However, why would we treat a __free variable any differently from one
> without the annotation?  The only difference is that a function gets
> called on it before exit, but as long as something can detect calling
> this on uninitialized variables their properties are definitely no
> different from non-__free variables so the can be treated exactly the
> same.
> 
> To revisit why we do this for non-__free variables: most people
> (although there are definitely languages where this isn't true and
> people who think we should follow this) think that having variables at
> the top of a function (or at least top of a code block) make the code
> easier to understand.  Additionally, keeping the variable uninitialized
> allows the compiler to detect any use before set scenarios, which can
> be somewhat helpful detecting code faults (I'm less persuaded by this,
> particularly given the number of false positive warnings we've seen
> that force us to add annotations, although this seems to be getting
> better).
> 
> So either we throw out the above for everything ... which I really
> wouldn't want, or we enforce it for *all* variables.
> 

Yeah.  You make a good point...

On the other hand, a bunch of maintainers are convinced that every free
variable should be initialized to a valid value at declaration time and
will reject patches which don't do that.  I see checkpatch as a way of
avoiding this round trip where a patch is automatically rejected because
of something trivial.

The truth is that the cleanup.h stuff is really new and I don't think
we've necessarily figured out all the best practices yet.

regards,
dan carpenter