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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <b051f580-a6e0-bd32-a6ad-2d172de3aecc@linux.intel.com>
Date: Thu, 5 Dec 2024 15:18:20 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Kurt Borja <kuurtb@...il.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, 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....

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