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: <bed8636bc4d036f4b2fe532e7bb4bb4b05c059fc.camel@HansenPartnership.com>
Date: Thu, 06 Nov 2025 11:06:29 -0500
From: James Bottomley <James.Bottomley@...senPartnership.com>
To: Dan Carpenter <dan.carpenter@...aro.org>
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, 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.

Regards,

James