| 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
| ||
|
Message-ID: <20250724182611.GC1266901@horms.kernel.org>
Date: Thu, 24 Jul 2025 19:26:11 +0100
From: Simon Horman <horms@...nel.org>
To: Jimmy Assarsson <extja@...ser.com>
Cc: linux-can@...r.kernel.org, Jimmy Assarsson <jimmyassarsson@...il.com>,
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 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.
That is, do this:
diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
index 758fd13f1bf4..a4402b4845c6 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
@@ -1974,6 +1974,7 @@ static int kvaser_usb_hydra_set_led(struct kvaser_usb_net_priv *priv,
{
struct kvaser_usb *dev = priv->dev;
struct kvaser_cmd *cmd;
+ size_t cmd_len;
int ret;
cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
@@ -1981,6 +1982,7 @@ static int kvaser_usb_hydra_set_led(struct kvaser_usb_net_priv *priv,
return -ENOMEM;
cmd->header.cmd_no = CMD_LED_ACTION_REQ;
+ cmd_len = kvaser_usb_hydra_cmd_size(cmd);
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));
@@ -1990,7 +1992,7 @@ static int kvaser_usb_hydra_set_led(struct kvaser_usb_net_priv *priv,
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));
+ ret = kvaser_usb_send_cmd(dev, cmd, cmd_len);
kfree(cmd);
return ret;
> + kfree(cmd);
> +
> + return ret;
> +}
...
Powered by blists - more mailing lists