Re: [PATCH v4 3/3] media: iris: Add Gen2 firmware autodetect and fallback

[Bryan O'Donoghue <bod@kernel.org>]

On 06/06/2026 11:51, Dmitry Baryshkov wrote:
>> version = strnstr(data, marker, size);
> strnstr is defined to search for the substring in another string. There
> is no promise that it will work if data contains \0 chars (which would
> terminate the string).

I mean... we'd want to terminate on a NULL here I'd have thought. The 
subsequent strscpy, strncmp and sscanf in this routine would imply NULL 
termination.

No wait I see - the strscpy() in the original creates a putatively NULL 
terminated string from potentially non-NULL terminated data.


The key question is if this is a NULL terminated string or not. If not 
we would expect a header somewhere telling us the field length.

>> if (version) {
>> 	marker_off = version - data;
>> 	version += marker_len;
>> 	size -= marker_off + marker_len;
>>
>> 	if (version < terminator-3) {
>> 		/* This is safe because we bounds check */
>> 		if (strncmp("vfw", version, size) == 0)
>> 			return true;
>> 	}
>>
>> 	/* To do your sscanf() you need to create a zeorised buffer */
>> 	fat_buf = kzalloc(size+1, GFP_KERNEL);
>> 	if (!fat_buf)
>> 		return false;
>>
>> 	memcpy(fat_buf, version, size);
> Creating a copy of about the half of the image is definitely an
> overkill.

The image size part I wasn't sure about - were we dealing with a defined 
header or the _entire_ image with the given size.

That said - why are we scanning the entire image if size == sizeof(fw) 
anyway ?

There must be a maximum header size and if not a maximum we would be 
prepared to parse in-kernel - say the first 1k or 4k at max.

...
>> 	/* sscanf is now guaranteed to terminate on NULL */
>> 	if (sscanf(fat_buf, "video-firmware.%d.%d", &major, &minor) == 2) {
> I think we can replace this with string comparisons too. No need to
> sscanf it.
> 
> WDYT?

I'm just as happy with that. It was really this code "looked wrong" and 
so I dug into it a little bit.

The overflow is real. The size you pointed out is true. Take the 
suggested changes with a pinch of salt.

This is the part that really pinged me

+	for (size_t i = 0; i < max; i++) {
+		if (!memcmp(data + i, marker, mlen)) {

Iterating a string for a memcmp() instead of using standard string libs, 
only then when I looked did I see the overflow.

So long as that gets fixed I'm sanguine about the rest.

---
bod