Re: [PATCH v2] mfd: viperboard: fix native fields type in structures as little-endiand

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [Low] The commit message description is not capitalized.

Pre-existing issues:
- [Critical] The DMA buffer `buf` in `struct vprbrd` shares a cacheline with `struct mutex lock`, leading to potential memory corruption during USB IN transfers.
- [High] The return value of i2c_add_adapter() is not checked in the probe function.
- [High] vprbrd_i2c_xfer() returns 0 on success instead of the number of messages successfully processed.
- [High] vprbrd_i2c_receive() uses the same struct pointer for the outgoing request header and the incoming data buffer, overwriting the header and breaking multi-chunk reads.
--

--- Patch [1]: [PATCH v2] mfd: viperboard: fix native fields type in structures as little-endiand ---
commit a510e96109198a25ffd2275333a8ed6cfaf2b401
Author: Mikhail Lukianchikov <[email protected]>

mfd: viperboard: fix native fields type in structures as little-endiand

This commit updates several structure fields in viperboard.h to use
little-endian types to resolve sparse warnings.

> mfd: viperboard: fix native fields type in structures as little-endiand

[Severity: Low]
Does this commit message description violate the MFD subsystem formatting
guidelines by not capitalizing the description after the prefix?

> diff --git a/include/linux/mfd/viperboard.h b/include/linux/mfd/viperboard.h
> index 193452848c047..de2be92bc2e89 100644
> --- a/include/linux/mfd/viperboard.h
> +++ b/include/linux/mfd/viperboard.h

[ ... ]

> @@ -94,7 +94,7 @@ struct vprbrd_i2c_addr_msg {
>  	u8 cmd;
>  	u8 addr;
>  	u8 unknown1;
> -	u16 len;
> +	__le16 len;
>  	u8 unknown2;
>  	u8 unknown3;
>  } __packed;

[Severity: Critical]
This is a pre-existing issue, but does this struct definition later in the
file share a cacheline between the DMA buffer and the mutex?

include/linux/mfd/viperboard.h:struct vprbrd {
    ...
	struct mutex lock;
	u8 buf[sizeof(struct vprbrd_i2c_write_msg)];
    ...
}

If a USB IN transfer is initiated in vprbrd_i2c_receive(), it maps
this unaligned buffer for DMA.

Wouldn't a concurrent modification to the mutex (like adding to the wait
queue) be discarded by the CPU's cacheline invalidation on
non-cache-coherent architectures, corrupting the mutex state and causing
a panic?

[Severity: High]
This isn't a bug introduced by this patch, but doesn't this code ignore the
mandatory return value of i2c_add_adapter() during probe?

drivers/i2c/busses/i2c-viperboard.c:vprbrd_i2c_probe() {
    ...
	/* attach to i2c layer */
	i2c_add_adapter(&vb_i2c->i2c);

	platform_set_drvdata(pdev, vb_i2c);

	return 0;
}

If i2c_add_adapter() fails, the adapter is not registered, but probe() still
returns 0 (success). When the driver is later unbound, won't
vprbrd_i2c_remove() unconditionally call i2c_del_adapter() on the
unregistered adapter and crash the system?

[Severity: High]
This is a pre-existing issue, but does vprbrd_i2c_xfer() return 0 on success
instead of the number of messages processed?

drivers/i2c/busses/i2c-viperboard.c:vprbrd_i2c_xfer() {
    ...
		mutex_unlock(&vb->lock);
	}
	return 0;
error:
	mutex_unlock(&vb->lock);
	return error;
}

The I2C subsystem requires the master_xfer callback to return the number of
successfully transferred messages. 

Doesn't returning 0 cause callers like i2c_transfer() to interpret a
successful transfer as a failure, breaking I2C client drivers attached to
this bus?

[Severity: High]
This isn't a bug introduced by this patch, but does vprbrd_i2c_receive() use
the same struct pointer for the outgoing request header and the incoming data
buffer?

drivers/i2c/busses/i2c-viperboard.c:vprbrd_i2c_receive() {
    ...
	/* read the actual data */
	ret = usb_bulk_msg(usb_dev,
		usb_rcvbulkpipe(usb_dev, VPRBRD_EP_IN), rmsg,
		bytes_xfer, &bytes_actual, VPRBRD_USB_TIMEOUT_MS);
    ...
}

It receives incoming USB payload data directly into the rmsg pointer at
offset 0, which overwrites rmsg->header.

If an I2C read message has len > 512, doesn't it trigger the second chunk
transfer in vprbrd_i2c_read() and transmit the corrupted rmsg->header to the
device?

drivers/i2c/busses/i2c-viperboard.c:vprbrd_i2c_read() {
    ...
		/* second read transfer if neccessary */
		if (len2 > 0) {
			ret = vprbrd_i2c_receive(vb->usb_dev, rmsg, len2);
    ...
}

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