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

[Kurt Borja]

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@xxxxxxxxx>
> 
> > > >  	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.