Re: [PATCH RFC v5 5/6] iio: osf: add UART transport

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The frame decoder validates header fields before verifying the frame CRC, returning `-EPROTO` for structural errors and causing the stream parser to drop unauthenticated payload lengths.
--

commit ca765586fd1687d99b4b8cd4c1d18456513a142b
Author: Jinseob Kim <kimjinseob88@gmail.com>

iio: osf: add UART transport

Add the serdev UART transport and the initial OSF core receive path.
Enable the required vcc regulator with devm_regulator_get_enable()
before opening the UART, keeping power handling limited to the simple
probe-time requirement for this RFC.

> diff --git a/drivers/iio/opensensorfusion/osf_core.c b/drivers/iio/opensensorfusion/osf_core.c
> new file mode 100644
> index 0000000000000..137fb7166febc
> --- /dev/null
> +++ b/drivers/iio/opensensorfusion/osf_core.c
> @@ -0,0 +1,99 @@
[ ... ]
> +int osf_core_receive_frame(struct osf_device *osf, const u8 *buf, size_t len)
> +{
> +	struct osf_frame frame;
> +	size_t frame_len;
> +	int ret;
> +
> +	if (!osf || !buf)
> +		return -EINVAL;
> +
> +	ret = osf_protocol_decode_frame(buf, len, &frame, &frame_len);

[Severity: High]
Could this error handling cause a regression where data is lost during parsing?

osf_protocol_decode_frame() validates header fields (like the major version
and header length) before checking the CRC, returning -EPROTO on structural
errors:

drivers/iio/opensensorfusion/osf_protocol.c:osf_protocol_decode_frame() {
    ...
	major = buf[4];
	if (major != OSF_PROTOCOL_MAJOR)
		return -EPROTO;

	if (get_unaligned_le16(buf + 6) != OSF_FRAME_HEADER_LEN)
		return -EPROTO;
    ...
}

If -EPROTO is returned here, the stream parser discards the unauthenticated
frame_len number of bytes instead of dropping 1 byte for resynchronization:

drivers/iio/opensensorfusion/osf_stream.c:osf_stream_process() {
    ...
		ret = osf_core_receive_frame(stream->osf, stream->buf, frame_len);
		if (ret) {
			if (ret == -EBADMSG) {
				stream->stats.bad_crc_frames++;
				stream->stats.dropped_bytes++;
				osf_stream_drop_invalid_head(stream);
			} else {
				osf_stream_discard(stream, frame_len);
			}
    ...
}

Because the CRC wasn't evaluated yet, frame_len is derived from an
unauthenticated payload_len, which could be a large, corrupted value. This
regression might cause the stream parser to skip legitimate frames hidden in
the incoming buffer under noisy conditions.

> +	if (ret)
> +		return ret;
> +
> +	if (frame_len != len)
> +		return -EMSGSIZE;
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616072242.3942-1-kimjinseob88@gmail.com?part=5