linux-kernel - Re: [PATCH v2 3/7] leds: add driver for LEDs from qnap-mcu devices

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: <2916408.FA0FI3ke8A@diego>
Date: Mon, 29 Jul 2024 09:48:50 +0200
From: Heiko Stübner <heiko@...ech.de>
To: Florian Eckert <fe@....tdt.de>
Cc: lee@...nel.org, jdelvare@...e.com, linux@...ck-us.net,
 dmitry.torokhov@...il.com, pavel@....cz, robh@...nel.org, krzk+dt@...nel.org,
 conor+dt@...nel.org, ukleinek@...ian.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-rockchip@...ts.infradead.org, linux-hwmon@...r.kernel.org,
 linux-input@...r.kernel.org, linux-leds@...r.kernel.org
Subject: Re: [PATCH v2 3/7] leds: add driver for LEDs from qnap-mcu devices

Hi Florian,

Am Montag, 29. Juli 2024, 08:24:26 CEST schrieb Florian Eckert:
> > +static int qnap_mcu_register_err_led(struct device *dev, struct
> > qnap_mcu *mcu, int num)
> > +{
> > +	struct qnap_mcu_err_led *err_led;
> > +	char tmp_buf[LED_MAX_NAME_SIZE];
> > +	int ret;
> > +
> > +	err_led = devm_kzalloc(dev, sizeof(*err_led), GFP_KERNEL);
> > +	if (!err_led)
> > +		return -ENOMEM;
> > +
> > +	err_led->mcu = mcu;
> > +	err_led->num = num;
> > +	err_led->mode = QNAP_MCU_ERR_LED_OFF;
> > +
> > +	snprintf(tmp_buf, LED_MAX_NAME_SIZE, "hdd%d:red:status", num + 1);
> > +	err_led->cdev.name = tmp_buf;
> 
> Should not the memory have to be allocated here via 'kzalloc' for 
> 'err_led->cdev.name'?
> After leaving the function, tmp_buf is no longer on the stack?

Reading the led_classdev_register thing, cdev->name is used only for
creating the final-name for the LED and thus copied into yet another
temporary buffer [0] .

And cdev->name is not accessed anymore outside the register function.

But thinking more about that, you're still right, because after registering
cdev->name is in a bad state, pointing to something no valid anymore.
So if the LED core changes behaviour in the future, this will cause breakage.

So I'll change that.


Thanks for catching that
Heiko



[0] https://elixir.bootlin.com/linux/v6.10.1/source/drivers/leds/led-class.c#L500
https://elixir.bootlin.com/linux/v6.10.1/source/drivers/leds/led-class.c#L503