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

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;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

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