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

[Ilpo Järvinen]

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

> > > -	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?

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