linux-kernel - Re: [RFC PATCH 05/21] alienware-wmi: Refactor rgb-zones sysfs group creation

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 for Android: free password hash cracker in your pocket

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

Message-ID: <nsx7vz5qtbxo5vq3ukzhfopkyrsi2bnhhelfpvxolqqdiokg25@6bgy5nclgftc>
Date: Thu, 5 Dec 2024 10:34:54 -0300
From: Kurt Borja <kuurtb@...il.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Dell.Client.Kernel@...l.com, Hans de Goede <hdegoede@...hat.com>, 
	LKML <linux-kernel@...r.kernel.org>, mario.limonciello@....com, platform-driver-x86@...r.kernel.org, 
	w_armin@....de
Subject: Re: [RFC PATCH 05/21] alienware-wmi: Refactor rgb-zones sysfs group
 creation

On Thu, Dec 05, 2024 at 03:18:20PM +0200, Ilpo Järvinen wrote:
> On Thu, 5 Dec 2024, Kurt Borja wrote:
> 
> > On Thu, Dec 05, 2024 at 12:17:01PM +0200, Ilpo Järvinen wrote:
> > > On Wed, 4 Dec 2024, Kurt Borja wrote:
> > > 
> > > > Define zone_attrs statically with the use of helper macros and
> > > > initialize the zone_attribute_group with driver's .dev_groups.
> > > > 
> > > > This makes match_zone() no longer needed, so drop it.
> > > > 
> > > > Signed-off-by: Kurt Borja <kuurtb@...il.com>
> 
> > > >  	zone_data =
> > > >  	    kcalloc(quirks->num_zones, sizeof(struct platform_zone),
> > > >  		    GFP_KERNEL);
> 
> You kcalloc() zone_data here for quirks->num_zones entries....

Totally missed this, thanks. 

> 
> > > > -	for (zone = 0; zone < quirks->num_zones; zone++) {
> > > > -		name = kasprintf(GFP_KERNEL, "zone%02hhX", zone);
> > > > -		if (name == NULL)
> > > > -			return 1;
> > > > -		sysfs_attr_init(&zone_dev_attrs[zone].attr);
> > > > -		zone_dev_attrs[zone].attr.name = name;
> > > > -		zone_dev_attrs[zone].attr.mode = 0644;
> > > > -		zone_dev_attrs[zone].show = zone_show;
> > > > -		zone_dev_attrs[zone].store = zone_set;
> > > > +	for (zone = 0; zone < 4; zone++)
> > > >  		zone_data[zone].location = zone;
> > > 
> > > You allocate quirks->num_zones entries to zone_data above but use a 
> > > literal here?
> > 
> > I did this because quirks->num_zones controlls only visibility now.
> 
> This kind of information would be useful to put into the commit message!
> 
> It does not control only visibility, see the kcalloc() code above. Did you 
> mean to alter the alloc too but forgot?

Yes I did not pay much attention when modifying this function. I'll fix
it.

> 
> > I didn't feel comfortable leaving an out of bounds access on zone_show()
> > and zone_set() when they do `zone_data[location]`.
> > 
> > Still those out of bounds accesses are hidden from user-space (right?) and
> > alienware_wmi_init() is getting dropped anyway so I should just leave it
> > as zone < quirks->num_zones.
> 
> The assignment within this loop will write out of bounds if 
> quirks->num_zones is less than 4.
> 
> -- 
>  i.