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: <407aed0ff7be4fdcafebd09e58e25496b6b4fec0.camel@HansenPartnership.com>
Date: Wed, 05 Nov 2025 10:32:19 -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 Wed, 2025-11-05 at 17:46 +0300, Dan Carpenter wrote:
> On Wed, Nov 05, 2025 at 09:21:45AM -0500, James Bottomley wrote:
> > On Wed, 2025-11-05 at 19:44 +0530, Ally Heev wrote:
> > > Uninitialized pointers with `__free` attribute can cause
> > > undefined
> > > behaviour as the memory assigned(randomly) to the pointer is
> > > freed
> > > automatically when the pointer goes out of scope
> > > 
> > > scsi doesn't have any bugs related to this as of now, but
> > > it is better to initialize and assign pointers with `__free` attr
> > > in one statement to ensure proper scope-based cleanup
> > > 
> > > Reported-by: Dan Carpenter <dan.carpenter@...aro.org>
> > > Closes:
> > > https://lore.kernel.org/all/aPiG_F5EBQUjZqsl@stanley.mountain/
> > > Signed-off-by: Ally Heev <allyheev@...il.com>
> > > ---
> > >  drivers/scsi/scsi_debug.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/scsi/scsi_debug.c
> > > b/drivers/scsi/scsi_debug.c
> > > index
> > > b2ab97be5db3d43d5a5647968623b8db72448379..89b36d65926bdd15c0ae93a
> > > 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.

Regards,

James