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

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

[Rahul Kumar]

Replace strncpy with memcpy in sm_attr_show and explicitly add a NUL
terminator after the copy. Also update the return value to reflect the
extra byte written for the terminator. This aligns with current kernel
best practices as strncpy is deprecated for such use, as explained in
Documentation/process/deprecated.rst.

No functional change, only cleanup for consistency.

Suggested-by: Pratyush Yadav <pratyush@kernel.org>
Signed-off-by: Rahul Kumar <rk0006818@gmail.com>
---
Changes in v1:
- Update return value to match the extra NUL written.
Link to v1: https://lore.kernel.org/all/mafs0ms7bvcd2.fsf@kernel.org/T/#t
---
 drivers/mtd/sm_ftl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/sm_ftl.c b/drivers/mtd/sm_ftl.c
index d28d4f1790f5..3c5d6d0c728f 100644
--- a/drivers/mtd/sm_ftl.c
+++ b/drivers/mtd/sm_ftl.c
@@ -44,8 +44,9 @@ static ssize_t sm_attr_show(struct device *dev, struct device_attribute *attr,
 	struct sm_sysfs_attribute *sm_attr =
 		container_of(attr, struct sm_sysfs_attribute, dev_attr);
 
-	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;
 }
 
 
-- 
2.43.0


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

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

[Miquel Raynal]

Hi Rahul,

On 08/09/2025 at 12:31:24 +0530, Rahul Kumar <rk0006818@gmail.com> wrote:

> Replace strncpy with memcpy in sm_attr_show and explicitly add a NUL
> terminator after the copy. Also update the return value to reflect the
> extra byte written for the terminator. This aligns with current kernel
> best practices as strncpy is deprecated for such use, as explained in
> Documentation/process/deprecated.rst.

The doc states "strscpy" as a replacement, not memcpy.

> No functional change, only cleanup for consistency.
>
> Suggested-by: Pratyush Yadav <pratyush@kernel.org>
> Signed-off-by: Rahul Kumar <rk0006818@gmail.com>
> ---
> Changes in v1:
> - Update return value to match the extra NUL written.
> Link to v1: https://lore.kernel.org/all/mafs0ms7bvcd2.fsf@kernel.org/T/#t
> ---
>  drivers/mtd/sm_ftl.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/sm_ftl.c b/drivers/mtd/sm_ftl.c
> index d28d4f1790f5..3c5d6d0c728f 100644
> --- a/drivers/mtd/sm_ftl.c
> +++ b/drivers/mtd/sm_ftl.c
> @@ -44,8 +44,9 @@ static ssize_t sm_attr_show(struct device *dev, struct device_attribute *attr,
>  	struct sm_sysfs_attribute *sm_attr =
>  		container_of(attr, struct sm_sysfs_attribute, dev_attr);
>  
> -	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?

Thanks,
Miquèl

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

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

[Richard Weinberger]

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

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(), ...

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.

Thanks,
//richard

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

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

[Pratyush Yadav]

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/

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

[Rahul Kumar]

Hi all,

Thanks a lot for the detailed feedback on my patch. I understand now
that this change does not add much value, and I’ll keep your points in
mind for future contributions. I really appreciate the guidance.

Best,
Rahul

On Tue, Sep 9, 2025 at 5:15 PM Pratyush Yadav <pratyush@kernel.org> wrote:
>
> 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/

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

[Richard Weinberger]

Rahul,

----- Ursprüngliche Mail -----
> Von: "Rahul Kumar" <rk0006818@gmail.com>
> Thanks a lot for the detailed feedback on my patch. I understand now
> that this change does not add much value, and I’ll keep your points in
> mind for future contributions. I really appreciate the guidance.

I hope my comment wasn't too demotivating.
You are *very* welcome to improve the code.
One of the major problems in open-source projects is that
we're short on review power. That's why sometimes maintainers (including me)
react grumpily when code is changed just for the sake of change.

Thanks,
//richard

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

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

[Rahul Kumar]

Hi Richard,

I sincerely appreciate your encouraging words. They motivate me to
continue improving and making better contributions.

Best regards,
Rahul Kumar


On Wed, Sep 10, 2025 at 1:20 AM Richard Weinberger <richard@nod.at> wrote:
>
> Rahul,
>
> ----- Ursprüngliche Mail -----
> > Von: "Rahul Kumar" <rk0006818@gmail.com>
> > Thanks a lot for the detailed feedback on my patch. I understand now
> > that this change does not add much value, and I’ll keep your points in
> > mind for future contributions. I really appreciate the guidance.
>
> I hope my comment wasn't too demotivating.
> You are *very* welcome to improve the code.
> One of the major problems in open-source projects is that
> we're short on review power. That's why sometimes maintainers (including me)
> react grumpily when code is changed just for the sake of change.
>
> Thanks,
> //richard

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

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

[Pratyush Yadav]

Hi Rahul,

On Mon, Sep 08 2025, Rahul Kumar wrote:

> Replace strncpy with memcpy in sm_attr_show and explicitly add a NUL
> terminator after the copy. Also update the return value to reflect the
> extra byte written for the terminator. This aligns with current kernel
> best practices as strncpy is deprecated for such use, as explained in
> Documentation/process/deprecated.rst.
>
> No functional change, only cleanup for consistency.
>
> Suggested-by: Pratyush Yadav <pratyush@kernel.org>

A Suggested-by tag indicates that the patch idea was suggested by that
person. In this case, I did not suggest this patch and merely reviewed
it. So a Suggested-by is slightly odd here. You can read more about the
tags here:
https://docs.kernel.org/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes

[...]

-- 
Regards,
Pratyush Yadav

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