Re: [PATCH v4 5/7] can: rcar_canfd: Add support for Renesas RZ/G3S

[Claudiu Beznea <claudiu.beznea+renesas@tuxon.dev>]

On 9/2/26 17:30, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [High] Lack of validation for Classical CAN mode on CAN FD-only hardware.
> 
> Pre-existing issues:
> - [High] Dangling pointer in global array leads to double free when register_candev fails.
> --
> 
> commit 1c2240055c546ef67856ad4716f6c425dbce01c6
> Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> can: rcar_canfd: Add support for Renesas RZ/G3S
> 
> This commit adds support for the Renesas RZ/G3S CAN-FD controller.
> The RZ/G3S controller supports two channels, 16 AFL pages, and operates
> exclusively in CAN-FD mode.
> 
>> diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
>> index bed7cde1aa01b..48e0573a73a44 100644
>> --- a/drivers/net/can/rcar/rcar_canfd.c
>> +++ b/drivers/net/can/rcar/rcar_canfd.c
> [ ... ]
>> @@ -661,6 +661,26 @@ static const struct rcar_canfd_hw_info rzg2l_hw_info = {
>>   	.fixed_canfd = 0,
>>   };
>>   
>> +static const struct rcar_canfd_hw_info r9a08g045_hw_info = {
>> +	.nom_bittiming = &rcar_canfd_gen4_nom_bittiming_const,
>> +	.data_bittiming = &rcar_canfd_gen4_data_bittiming_const,
>> +	.tdc_const = &rcar_canfd_gen4_tdc_const,
>> +	.regs = &rcar_gen4_regs,
>> +	.sh = &rcar_gen4_shift_data,
>> +	.rnc_field_width = 16,
>> +	.max_aflpn = 15,
>> +	.max_cftml = 31,
>> +	.max_channels = 2,
>> +	.postdiv = 2,
>> +	.shared_global_irqs = 0,
>> +	.multi_channel_irqs = 1,
>> +	.ch_interface_mode = 1,
>> +	.shared_can_regs = 1,
>> +	.external_clk = 1,
>> +	.fcan_pclk = 1,
>> +	.fixed_canfd = 1,
> 
> [Severity: High]
> If the device tree specifies the "renesas,no-can-fd" property, could this
> lead to a silent mismatch between the software network stack configuration
> and the hardware state?

The DT schema was updated to forbid renesas,no-can-fd for RZ/G3S. AFAIK, we 
shouldn't validate the device tree in kernel. That's should be done by the 
dtbs_check.

> 
> In drivers/net/can/rcar/rcar_canfd.c:rcar_canfd_probe(), if the property is
> present, the driver will set fdmode = false:
> 
> 	if (of_property_read_bool(dev->of_node, "renesas,no-can-fd"))
> 		fdmode = false;
> 
> However, when rcar_canfd_configure_controller() runs, it skips the
> controller mode update because fixed_canfd is true:
> 
> 	if (gpriv->info->fixed_canfd)
> 		continue;
> 
> This effectively leaves the hardware locked in CAN FD mode, while the rest of
> the driver initializes the software stack for Classical CAN. For example,
> rcar_canfd_channel_probe() initializes a max MTU of 16 bytes, and
> rcar_canfd_configure_rx() restricts the physical hardware Rx FIFO payload
> size to 8 bytes.
> 
> If the hardware receives a 64-byte CAN FD frame, will it attempt to write
> 64 bytes into an Rx FIFO configured for 8 bytes, causing a buffer overflow?
> 
>> +};
>> +
> [ ... ]
>> @@ -2383,6 +2403,7 @@ static DEFINE_SIMPLE_DEV_PM_OPS(rcar_canfd_pm_ops, rcar_canfd_suspend,
>>   
>>   static const __maybe_unused struct of_device_id rcar_canfd_of_table[] = {
>>   	{ .compatible = "renesas,r8a779a0-canfd", .data = &rcar_gen4_hw_info },
>> +	{ .compatible = "renesas,r9a08g045-canfd", .data = &r9a08g045_hw_info },
> 
> [Severity: High]
> This is a pre-existing issue, but does probe failure on this newly added
> hardware expose a potential double-free?
> 
> In drivers/net/can/rcar/rcar_canfd.c:rcar_canfd_channel_probe(), the global
> array is populated before device registration:
> 
> 	gpriv->ch[priv->channel] = priv;
> 	...
> 	err = register_candev(ndev);
> 	if (err) {
> 		...
> 		goto fail_candev;
> 	}
> 
> If register_candev() fails, the error path calls free_candev(ndev), which
> also frees the embedded priv structure, but leaves the dangling pointer in
> gpriv->ch[priv->channel].
> 
> When the failure propagates back to rcar_canfd_probe(), the cleanup block
> iterates over all configured channels and calls rcar_canfd_channel_remove()
> for each, including the one that just failed:
> 
> drivers/net/can/rcar/rcar_canfd.c:rcar_canfd_channel_remove() {
> 	...
> 	unregister_candev(priv->ndev);
> 	...
> 	free_candev(priv->ndev);
> }
> 
> Could this lead to memory corruption when it attempts to unregister and free
> the already freed dangling pointer?
> 
>>   	{ .compatible = "renesas,r9a09g047-canfd", .data = &r9a09g047_hw_info },
>>   	{ .compatible = "renesas,r9a09g077-canfd", .data = &r9a09g077_hw_info },
>>   	{ .compatible = "renesas,rcar-gen3-canfd", .data = &rcar_gen3_hw_info },
>