Re: [PATCH v5 5/5] tools: i3c: add i3ctransfer utility

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] tools/i3c: missing per-message mode parsing in getopt second pass
- [Medium] tools/i3c: silent data truncation of write payloads exceeding 255 bytes
- [Low] tools/i3c: potential heap out-of-bounds read in print_rx_data
--

commit 2852ade0c3821ed20131d9f8a5931798c8b93c03
Author: Sam Agazaryan <samagazaryan@google.com>

tools: i3c: add i3ctransfer utility

This adds the i3ctransfer utility for interacting with I3C target devices.
It supports SDR and HDR transfer modes and reports the actual length of
read bytes.

> diff --git a/tools/i3c/i3ctransfer.c b/tools/i3c/i3ctransfer.c
> new file mode 100644
> index 0000000000000..a5b071e635306
> --- /dev/null
> +++ b/tools/i3c/i3ctransfer.c

[ ... ]

> +static int w_args_to_xfer(struct i3c_ioc_xfer *xfer, char *arg,
> +			  uint8_t mode, int cmd)
> +{
> +	char *data_ptrs[256];
> +	int len, i = 0;
> +	uint8_t *tmp;
> +
> +	if (mode == I3C_XFER_MODE_SDR) {
> +		xfer->rnw = I3C_DEV_DIR_WRITE;
> +	} else {
> +		if (cmd < 0 || (cmd & 0x80)) {
> +			fprintf(stderr,
> +				"Error: HDR write requires command (-c) in range 0x00..0x7f\n");
> +			return -1;
> +		}
> +		xfer->cmd = (uint8_t)cmd;
> +	}
> +
> +	data_ptrs[i] = strtok(arg, ",");
> +	while (data_ptrs[i] && i < 255)
> +		data_ptrs[++i] = strtok(NULL, ",");

[Severity: Medium]
Does this silently discard data if the user provides a comma-separated write
payload longer than 255 bytes? The loop bounds token parsing to 255 elements,
meaning any remaining bytes are dropped without returning an error. Should
this return an error if strtok() indicates more tokens remain?

> +
> +	if (i == 0) {
> +		fprintf(stderr, "Error: empty write data block\n");
> +		return -1;
> +	}
> +
> +	tmp = calloc(i, sizeof(uint8_t));
> +	if (!tmp)
> +		return -1;
> +
> +	for (len = 0; len < i; len++) {
> +		char *endptr;
> +		long val = strtol(data_ptrs[len], &endptr, 0);

[ ... ]

> +static void print_rx_data(const struct i3c_ioc_xfer *xfer)
> +{
> +	const uint8_t *tmp = (const uint8_t *)(uintptr_t)xfer->data;
> +	int i;
> +
> +	fprintf(stdout, "  received data (%u bytes):\n", xfer->actual_len);
> +	for (i = 0; i < xfer->actual_len; i++)
> +		fprintf(stdout, "    0x%02x\n", tmp[i]);
> +}

[Severity: Low]
Could this lead to an out-of-bounds read if the kernel returns an actual_len
that is larger than the requested xfer->len? Since tmp is allocated based on
the requested length, iterating up to actual_len might read past the end of
the buffer if a buggy driver or device returns an unexpected length.

[ ... ]

> +int main(int argc, char *argv[])
> +{

[ ... ]

> +	optind = 1;
> +	nxfers = 0;
> +	cmd = -1;
> +
> +	while ((opt = getopt_long(argc, argv, sopts, lopts, NULL)) != EOF) {
> +		switch (opt) {
> +		case 'h':
> +		case 'v':
> +		case 'd':
> +		case 'm':
> +			break;

[Severity: Medium]
Does skipping the 'm' argument in the second getopt_long pass break
mixed-mode transfers? If a user interleaves multiple transfers with
different -m flags, it appears the utility assigns the mode of the
very last -m flag to all transfers, because the second pass doesn't
update the local mode variable per message.

> +		case 'c':
> +			cmd = (int)strtol(optarg, NULL, 0);
> +			break;

[ ... ]

> +	if (ioctl(file, I3C_IOC_XFER(nxfers), xfers) < 0) {
> +		fprintf(stderr, "Error: transfer failed: %s\n", strerror(errno));
> +		ret = EXIT_FAILURE;
> +		goto err_free;
> +	}
> +
> +	for (i = 0; i < nxfers; i++) {
> +		bool is_read = (mode == I3C_XFER_MODE_SDR) ?
> +			       (xfers[i].rnw == I3C_DEV_DIR_READ) :
> +			       (xfers[i].cmd & 0x80);
> +
> +		fprintf(stdout, "Success on message %d\n", i);
> +		if (is_read)
> +			print_rx_data(&xfers[i]);
> +	}

[Severity: Medium]
Because of the issue above, the output loop here also checks the global
mode variable instead of xfers[i].mode. Could this cause the tool to
incorrectly evaluate the read/write status of individual messages during a
mixed-mode sequence?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921230603.2518652-1-samagazaryan@google.com?part=5

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c