Re: [PATCH v4 3/3] i3c: master: Validate GET CCC payload length and retry Direct GET once

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The Renesas I3C driver fails to set `rx_count` on completion, forcing `actual_len` to 0 and failing all GET CCC validations
- [Low] Dead code in `i3c_master_getstatus_locked()` due to missed update from `len` to `actual_len`
- [Low] The payload validation function evaluates a mathematically impossible and redundant condition
--

commit 3c7b45c2572b29be23202a83e0e0241b82a602bb
Author: Adrian Ng Ho Yin <[email protected]>

i3c: master: Validate GET CCC payload length and retry Direct GET once

This commit adds optional_bytes to struct i3c_ccc_cmd_payload to describe
variable-length GET CCC responses and validates payload lengths in
i3c_master_send_ccc_cmd_locked(). It also retries failed Direct GET CCCs
and updates I3C master drivers to set actual_len on successful GET transfers.

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index f1be38a640ca1..5741c3f80ed58 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c

[ ... ]

> +static int i3c_ccc_validate_payload_len(struct i3c_ccc_cmd *cmd)
> +{
> +	unsigned int i;
> +
> +	if (!cmd->rnw)
> +		return 0;
> +
> +	for (i = 0; i < cmd->ndests; i++) {
> +		struct i3c_ccc_cmd_payload *p = &cmd->dests[i].payload;
> +		u16 min_len;
> +
> +		if (p->optional_bytes > p->len)
> +			return -EINVAL;
> +
> +		if (p->actual_len > p->len)
> +			return -EIO;
> +
> +		if (!p->len)
> +			continue;
> +
> +		min_len = p->len - p->optional_bytes;
> +		if (p->actual_len < min_len ||
> +		    (!p->optional_bytes && p->actual_len != p->len))
> +			return -EIO;
> +	}
> +
> +	return 0;
> +}

[Severity: Low]
Does this evaluate a redundant condition? Since p->actual_len > p->len is
already caught above, when !p->optional_bytes is true, min_len equals
p->len. The left side of the OR captures all cases where actual_len < len.
The right side is only evaluated if actual_len == len, making
actual_len != p->len always false.

> @@ -1409,7 +1473,7 @@ static int i3c_master_getmwl_locked(struct i3c_master_controller *master,
>  	if (ret)
>  		goto out;
>  
> -	if (dest.payload.len != sizeof(*mwl)) {
> +	if (dest.payload.actual_len != sizeof(*mwl)) {
>  		ret = -EIO;
>  		goto out;
>  	}

[Severity: Low]
Was i3c_master_getstatus_locked() missed in these updates?

This patch changes drivers to write the received payload length to
actual_len instead of modifying len, but i3c_master_getstatus_locked()
still checks the unmodified length:

drivers/i3c/master.c:i3c_master_getstatus_locked() {
    ...
    if (dest.payload.len != sizeof(*getstatus)) {
        ret = -EIO;
        goto out;
    }
    ...
}

Since len is initialized to sizeof(*getstatus) and is no longer modified
by the drivers, does this make the error path unreachable dead code?

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

[Severity: High]
Can this cause all GET CCC operations to fail on this hardware?

This code assigns cmd->rx_count to actual_len, but it appears
cmd->rx_count is never incremented during
I3C_INTERNAL_STATE_CONTROLLER_COMMAND_READ.

Looking at renesas_i3c_resp_isr():

    if (NDBSTLV0_RDBLV(renesas_readl(i3c->regs, NDBSTLV0)) && !cmd->err)
        bytes_remaining = data_len - cmd->rx_count;
    
    i3c_readl_fifo(i3c->regs + NTDTBP0, cmd->rx_buf, bytes_remaining);

If rx_count remains 0, actual_len will always be 0. Will the new
i3c_ccc_validate_payload_len() logic then unconditionally reject the
payload with -EIO due to a short read?

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

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