netdev - Re: [PATCH v2 01/11] can: kvaser_usb: Add support to control CAN LEDs on device

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: <a07d995f-0fdf-4773-8cc4-4db6f72ce398@gmail.com>
Date: Fri, 25 Jul 2025 14:44:52 +0200
From: Jimmy Assarsson <jimmyassarsson@...il.com>
To: Simon Horman <horms@...nel.org>, Jimmy Assarsson <extja@...ser.com>
Cc: linux-can@...r.kernel.org, Marc Kleine-Budde <mkl@...gutronix.de>,
 Vincent Mailhol <mailhol.vincent@...adoo.fr>, netdev@...r.kernel.org
Subject: Re: [PATCH v2 01/11] can: kvaser_usb: Add support to control CAN LEDs
 on device

On 7/24/25 8:26 PM, Simon Horman wrote:
> On Thu, Jul 24, 2025 at 11:24:55AM +0200, Jimmy Assarsson wrote:
>> Add support to turn on/off CAN LEDs on device.
>>
>> Signed-off-by: Jimmy Assarsson <extja@...ser.com>
> 
> ...
> 
>> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
> 
> ...
> 
>> +static int kvaser_usb_hydra_set_led(struct kvaser_usb_net_priv *priv,
>> +				    enum kvaser_usb_led_state state,
>> +				    u16 duration_ms)
>> +{
>> +	struct kvaser_usb *dev = priv->dev;
>> +	struct kvaser_cmd *cmd;
>> +	int ret;
>> +
>> +	cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
>> +	if (!cmd)
>> +		return -ENOMEM;
>> +
>> +	cmd->header.cmd_no = CMD_LED_ACTION_REQ;
>> +	kvaser_usb_hydra_set_cmd_dest_he(cmd, dev->card_data.hydra.sysdbg_he);
>> +	kvaser_usb_hydra_set_cmd_transid(cmd, kvaser_usb_hydra_get_next_transid(dev));
>> +
>> +	cmd->led_action_req.duration_ms = cpu_to_le16(duration_ms);
>> +	cmd->led_action_req.action = state |
>> +				     FIELD_PREP(KVASER_USB_HYDRA_LED_IDX_MASK,
>> +						KVASER_USB_HYDRA_LED_YELLOW_CH0_IDX +
>> +						KVASER_USB_HYDRA_LEDS_PER_CHANNEL * priv->channel);
>> +
>> +	ret = kvaser_usb_send_cmd(dev, cmd, kvaser_usb_hydra_cmd_size(cmd));
> 
> When building this file with GCC 15.1.0 with KCFLAGS=-Warray-bounds
> I see the following:
> 
>    In file included from ./include/linux/byteorder/little_endian.h:5,
>                     from ./arch/x86/include/uapi/asm/byteorder.h:5,
>                     from ./include/linux/bitfield.h:12,
>                     from drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c:15:
>    In function 'kvaser_usb_hydra_cmd_size',
>        inlined from 'kvaser_usb_hydra_set_led' at drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c:1993:38:
>    drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c:532:65: warning: array subscript 'struct kvaser_cmd_ext[0]' is partly outside array bounds of 'unsigned char[32]' [-Warray-bounds=]
>      532 |                 ret = le16_to_cpu(((struct kvaser_cmd_ext *)cmd)->len);
>    ./include/uapi/linux/byteorder/little_endian.h:37:51: note: in definition of macro '__le16_to_cpu'
>       37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
>          |                                                   ^
>    drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c:532:23: note: in expansion of macro 'le16_to_cpu'
>      532 |                 ret = le16_to_cpu(((struct kvaser_cmd_ext *)cmd)->len);
>          |                       ^~~~~~~~~~~
>    In file included from ./include/linux/fs.h:46,
>                     from ./include/linux/compat.h:17,
>                     from ./arch/x86/include/asm/ia32.h:7,
>                     from ./arch/x86/include/asm/elf.h:10,
>                     from ./include/linux/elf.h:6,
>                     from ./include/linux/module.h:19,
>                     from ./include/linux/device/driver.h:21,
>                     from ./include/linux/device.h:32,
>                     from drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c:17:
>    In function 'kmalloc_noprof',
>        inlined from 'kzalloc_noprof' at ./include/linux/slab.h:1039:9,
>        inlined from 'kvaser_usb_hydra_set_led' at drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c:1979:8:
>    ./include/linux/slab.h:905:24: note: object of size 32 allocated by '__kmalloc_cache_noprof'
>      905 |                 return __kmalloc_cache_noprof(
>          |                        ^~~~~~~~~~~~~~~~~~~~~~~
>      906 |                                 kmalloc_caches[kmalloc_type(flags, _RET_IP_)][index],
>          |                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>      907 |                                 flags, size);
>          |                                 ~~~~~~~~~~~~
> 
> 	if (cmd->header.cmd_no == CMD_EXTENDED)
> 		ret = le16_to_cpu(((struct kvaser_cmd_ext *)cmd)->len);
> 
> GCC seems to know that:
> * cmd was allocated sizeof(*cmd) = 32 bytes
> * struct kvaser_cmd_ext is larger than this (96 bytes)
> 
> And it thinks that cmd->header.cmd_no might be CMD_EXTENDED.
> This is not true, becuae .cmd_no is set to CMD_LED_ACTION_REQ
> earlier in kvaser_usb_hydra_set_led. But still, GCC produces
> a big fat warning.
> 
> On the one hand we might say this is a shortcoming in GCC,
> a position I agree with. But on the other hand, we might follow
> the pattern used elsewhere in this file for similar functions,
> which seems to make GCC happy, I guess, and it is strictly a guess,
> because less context is needed for it to analyse things correctly.

Thanks for finding this!

Marc Kleine-Budde actually sorted this out for other commands some years ago [1],
but I had completely forgotten.

[1] https://lore.kernel.org/all/20221219110104.1073881-1-mkl@pengutronix.de

...