Re: [PATCH v2] mtd: sm_ftl: replace strncpy with memcpy

[Pratyush Yadav <pratyush@kernel.org>]

On Mon, Sep 08 2025, Richard Weinberger wrote:

> ----- Ursprüngliche Mail -----
>> Von: "Miquel Raynal" <miquel.raynal@bootlin.com>
>> An: "Rahul Kumar" <rk0006818@gmail.com>
>>> -	strncpy(buf, sm_attr->data, sm_attr->len);
>>> -	return sm_attr->len;
>>> +	memcpy(buf, sm_attr->data, sm_attr->len);
>>> +	buf[sm_attr->len] = '\0';
>>> +	return sm_attr->len + 1;
>> 
>> Are we sure the buffer is always sm_attr->len + 1 long?

Yeah, that was what I was wondering on the original patch too. Poking
around the code, I see in dev_attr_show() that:

	if (dev_attr->show)
		ret = dev_attr->show(dev, dev_attr, buf);
	if (ret >= (ssize_t)PAGE_SIZE) {
		printk("dev_attr_show: %pS returned bad count\n",
				dev_attr->show);
	}

So I suppose the buffer is PAGE_SIZE long, though the show() API is
kinda poor for not reporting the size explicitly. If we do really want
to make this safer, I suppose this is what should be checked for. The
strncpy with length being limited to the string length instead of buffer
length is completely useless.

Anyway, here sm_attr->data is (SM_SMALL_PAGE - SM_CIS_VENDOR_OFFSET + 1)
bytes long, which should add up to 168 bytes, which is perfectly safe to
just copy.

>
> Can we please just stop messing with perfectly fine code?
> I'm sick of the war on string functions.
> First we had to replace everything with strncpy(), then strlcpy(),
> then strscpy(), ...

I had a similar reaction initally too. But TBH if the patch made the
code actually safer I reckon it would be fine. Long term, these kind of
things can help prevent unexpected bugs. But here we don't even know how
large buf is so there is no point in using any of the fancy string
functions. A plain strcpy(buf, str) is just as "safe" as a
strncpy(buf, str, strlen(str)), or any other fancy variation.

So yeah, I don't think this patch does much...

That said, Rahul you seem like a new contributor. I would say don't be
too discouraged. Not every patch ends up going through. I have had my
fair share of rejected patches. Take this as a learning and keep looking
for other improvements :-)

>
> Don't get me wrong, I'm all for hardening code paths where
> strings are arbitrary input, but in many of these cases all strings
> are no input or already sanitized.

-- 
Regards,
Pratyush Yadav

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/