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

[Dmitry Baryshkov]

On Sat, Jun 06, 2026 at 12:13:46PM +0100, Bryan O'Donoghue wrote:
> 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.

No, it is documented to create a NULL-terminated string from a
NULL-terminated string.

> 
> 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.

ELF file definitely is not a NULL-terminated string.

> 
> > > 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.

Because nobody has expected that we'd need to parse firmware version
from it. So it's not a header. It is a string in the middle of the
firmware blob.

> 
> ...
> > > 	/* 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.

Ack


-- 
With best wishes
Dmitry