Re: [PATCH v5 2/5] i3c: master: Report actual GET CCC payload length on success

On 3/7/2026 7:06 pm, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] ADI and Cadence I3C master drivers spoof actual_len, hiding short reads and bypassing core validation.
> - [High] Renesas I3C master driver leaves rx_count at 0 for small transfers, breaking device enumeration.
> --
> 
> commit bc63836138deed26f2a332a0c5e19f4ac088d4ab
> Author: Adrian Ng Ho Yin <[email protected]>
> 
> i3c: master: Report actual GET CCC payload length on success
>      
> Set dests[].payload.actual_len on successful GET CCC transfers in I3C
> master drivers so the core can distinguish requested and received
> buffer lengths. Switch core GET helpers to use actual_len instead of
> len when interpreting GET CCC results.
> 
> [ ... ]
> 
>> diff --git a/drivers/i3c/master/adi-i3c-master.c b/drivers/i3c/master/adi-i3c-master.c
>> index 047081c9f0643..64735b4887261 100644
>> --- a/drivers/i3c/master/adi-i3c-master.c
>> +++ b/drivers/i3c/master/adi-i3c-master.c
> [ ... ]
>> @@ -360,6 +360,8 @@ static int adi_i3c_master_send_ccc_cmd(struct i3c_master_controller *m,
>>   		adi_i3c_master_unqueue_xfer(master, xfer);
>>   
>>   	cmd->err = adi_i3c_cmd_get_err(&xfer->cmds[0]);
>> +	if (!xfer->ret && cmd->rnw)
>> +		cmd->dests[0].payload.actual_len = cmd->dests[0].payload.len;
> 
> [Severity: High]
> Does this unconditionally set actual_len to the requested length, discarding
> the true hardware transfer size?
> 
> By blindly copying the requested length here, it seems this defeats the new GET
> CCC validation logic. If a device returns 0 bytes or a short read, the core
> validation could falsely pass, potentially causing the kernel to parse
> uninitialized or zero-padded memory as valid device capabilities.
> 
>>   
>>   	return xfer->ret;
>>   }
> [ ... ]
>> diff --git a/drivers/i3c/master/i3c-master-cdns.c b/drivers/i3c/master/i3c-master-cdns.c
>> index 6d221596ea357..36998ee4073d7 100644
>> --- a/drivers/i3c/master/i3c-master-cdns.c
>> +++ b/drivers/i3c/master/i3c-master-cdns.c
> [ ... ]
>> @@ -714,6 +714,8 @@ static int cdns_i3c_master_send_ccc_cmd(struct i3c_master_controller *m,
>>   
>>   	ret = xfer->ret;
>>   	cmd->err = cdns_i3c_cmd_get_err(&xfer->cmds[0]);
>> +	if (!ret && cmd->rnw)
>> +		cmd->dests[0].payload.actual_len = cmd->dests[0].payload.len;
> 
> [Severity: High]
> Similar to the ADI driver, does this bypass the core validation by
> unconditionally copying the requested length?
> 
> If a short read occurs, the driver will incorrectly report a full read instead
> of the actual hardware byte count.
> 
>>   	cdns_i3c_master_free_xfer(xfer);
>>   
>>   	return ret;
> [ ... ]
>> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
>> index f39c449922ca2..fec614700843f 100644
>> --- a/drivers/i3c/master/renesas-i3c.c
>> +++ b/drivers/i3c/master/renesas-i3c.c
> [ ... ]
>> @@ -805,6 +805,8 @@ static int renesas_i3c_send_ccc_cmd(struct i3c_master_controller *m,
>>   	ret = xfer->ret;
>>   	if (ret)
>>   		ccc->err = I3C_ERROR_M2;
>> +	else if (ccc->rnw)
>> +		ccc->dests[0].payload.actual_len = cmd->rx_count;
> 
> [Severity: High]
> Does this leave actual_len at 0 for small transfers, breaking device
> enumeration?
> 
> For small GET CCC reads, if the read buffer full interrupt doesn't
> trigger, the transfer completes in renesas_i3c_resp_isr() where it drains the
> remaining bytes from the FIFO but doesn't accumulate bytes_remaining into
> cmd->rx_count:
> 
> drivers/i3c/master/renesas-i3c.c:renesas_i3c_resp_isr() {
>      ...
>      bytes_remaining = data_len - cmd->rx_count;
>      i3c_readl_fifo(i3c->regs + NTDTBP0, cmd->rx_buf, bytes_remaining);
>      ...
> }
> 
> Since cmd->rx_count remains 0, won't this propagate an actual_len of 0 to the
> core, causing the core validation to reject the transfer with -EIO?
> 
>>   
>>   	return ret;
>>   }
> 

Hi maintainers,

Both points from Sashiko look valid.
1. [High] ADI and Cadence I3C master drivers spoof actual_len, hiding 
short reads and bypassing core validation.

We currently set actual_len from the requested payload.len rather than 
the hardware byte count (REG_CMDR_FIFO_XFER_BYTES / CMDR_XFER_BYTES), 
even though the completion path already reads that value to drain the RX 
FIFO. We'll fix this in v6 by recording the HW transfer size and using 
it for actual_len.

2. [High] Renesas I3C master driver leaves rx_count at 0 for small 
transfers, breaking device enumeration.

For small GET CCCs, rx_count may remain 0 when the read-buffer-full IRQ 
does not fire and resp_isr skips the FIFO drain because RDBLV is unset — 
while NRSPQP_DATA_LEN already reports the received length (ENTDAA 
handles this correctly). In v6, we'll align the COMMAND_READ path with 
ENTDAA so actual_len reflects data_len on success.

Let me know if you have different thoughts.

Thanks,
Tze Yee

-- 
linux-i3c mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c