[PATCH v2] uml: Replace strlcpy with strscpy

[PATCH v2] uml: Replace strlcpy with strscpy

[Azeem Shaikh]

strlcpy() reads the entire source buffer first.
This read may exceed the destination size limit.
This is both inefficient and can lead to linear read
overflows if a source string is not NUL-terminated [1].
In an effort to remove strlcpy() completely [2], replace
strlcpy() here with strscpy().
No return values were used, so direct replacement is safe.

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
[2] https://github.com/KSPP/linux/issues/89

Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com>
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202305311135.zGMT1gYR-lkp@intel.com/
---
v1: https://lore.kernel.org/all/20230530164004.986750-1-azeemshaikh38@gmail.com/

Changes from v1 - added strscpy declaration. v1 does not build.

 arch/um/include/shared/user.h          | 1 +
 arch/um/os-Linux/drivers/tuntap_user.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/um/include/shared/user.h b/arch/um/include/shared/user.h
index bda66e5a9d4e..0347a190429c 100644
--- a/arch/um/include/shared/user.h
+++ b/arch/um/include/shared/user.h
@@ -52,6 +52,7 @@ static inline int printk(const char *fmt, ...)
 extern int in_aton(char *str);
 extern size_t strlcpy(char *, const char *, size_t);
 extern size_t strlcat(char *, const char *, size_t);
+extern size_t strscpy(char *, const char *, size_t);
 
 /* Copied from linux/compiler-gcc.h since we can't include it directly */
 #define barrier() __asm__ __volatile__("": : :"memory")
diff --git a/arch/um/os-Linux/drivers/tuntap_user.c b/arch/um/os-Linux/drivers/tuntap_user.c
index 53eb3d508645..2284e9c1cbbb 100644
--- a/arch/um/os-Linux/drivers/tuntap_user.c
+++ b/arch/um/os-Linux/drivers/tuntap_user.c
@@ -146,7 +146,7 @@ static int tuntap_open(void *data)
 		}
 		memset(&ifr, 0, sizeof(ifr));
 		ifr.ifr_flags = IFF_TAP | IFF_NO_PI;
-		strlcpy(ifr.ifr_name, pri->dev_name, sizeof(ifr.ifr_name));
+		strscpy(ifr.ifr_name, pri->dev_name, sizeof(ifr.ifr_name));
 		if (ioctl(pri->fd, TUNSETIFF, &ifr) < 0) {
 			err = -errno;
 			printk(UM_KERN_ERR "TUNSETIFF failed, errno = %d\n",
-- 
2.41.0.rc0.172.g3f132b7071-goog

Re: [PATCH v2] uml: Replace strlcpy with strscpy

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "Azeem Shaikh" <azeemshaikh38@gmail.com>
> strlcpy() reads the entire source buffer first.
> This read may exceed the destination size limit.
> This is both inefficient and can lead to linear read
> overflows if a source string is not NUL-terminated [1].
> In an effort to remove strlcpy() completely [2], replace
> strlcpy() here with strscpy().
> No return values were used, so direct replacement is safe.
> 
> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> [2] https://github.com/KSPP/linux/issues/89
> 
> Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes:
> https://lore.kernel.org/oe-kbuild-all/202305311135.zGMT1gYR-lkp@intel.com/

Are you sure Reported-by and Closes make sense?
AFAIK the report was only on your first patch and nothing against upstream.
So stating this in the updated patch is in vain.

Other than that,
Acked-by: Richard Weinberger <richard@nod.at>

Thanks,
//richard

Re: [PATCH v2] uml: Replace strlcpy with strscpy

[Azeem Shaikh]

On Tue, Jun 6, 2023 at 4:51 PM Richard Weinberger <richard@nod.at> wrote:
>
> ----- Ursprüngliche Mail -----
> > Von: "Azeem Shaikh" <azeemshaikh38@gmail.com>
> > strlcpy() reads the entire source buffer first.
> > This read may exceed the destination size limit.
> > This is both inefficient and can lead to linear read
> > overflows if a source string is not NUL-terminated [1].
> > In an effort to remove strlcpy() completely [2], replace
> > strlcpy() here with strscpy().
> > No return values were used, so direct replacement is safe.
> >
> > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> > [2] https://github.com/KSPP/linux/issues/89
> >
> > Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com>
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes:
> > https://lore.kernel.org/oe-kbuild-all/202305311135.zGMT1gYR-lkp@intel.com/
>
> Are you sure Reported-by and Closes make sense?
> AFAIK the report was only on your first patch and nothing against upstream.
> So stating this in the updated patch is in vain.

I left the metadata in only for the sake of posterity. If it's not
helpful, I'm ok with removing it.

> Other than that,
> Acked-by: Richard Weinberger <richard@nod.at>

Thanks!

Re: [PATCH v2] uml: Replace strlcpy with strscpy

[Eric Biggers]

On Tue, Jun 06, 2023 at 05:08:27PM -0400, Azeem Shaikh wrote:
> On Tue, Jun 6, 2023 at 4:51 PM Richard Weinberger <richard@nod.at> wrote:
> >
> > ----- Ursprüngliche Mail -----
> > > Von: "Azeem Shaikh" <azeemshaikh38@gmail.com>
> > > strlcpy() reads the entire source buffer first.
> > > This read may exceed the destination size limit.
> > > This is both inefficient and can lead to linear read
> > > overflows if a source string is not NUL-terminated [1].
> > > In an effort to remove strlcpy() completely [2], replace
> > > strlcpy() here with strscpy().
> > > No return values were used, so direct replacement is safe.
> > >
> > > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> > > [2] https://github.com/KSPP/linux/issues/89
> > >
> > > Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com>
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Closes:
> > > https://lore.kernel.org/oe-kbuild-all/202305311135.zGMT1gYR-lkp@intel.com/
> >
> > Are you sure Reported-by and Closes make sense?
> > AFAIK the report was only on your first patch and nothing against upstream.
> > So stating this in the updated patch is in vain.
> 
> I left the metadata in only for the sake of posterity. If it's not
> helpful, I'm ok with removing it.
> 

IMO using Reported-by in cases like this is harmful, as it makes commits seem
like bug fixes when they are not.

- Eric

Reported-by/Closes tag for uncommitted issues (was: Re: [PATCH v2] uml: Replace strlcpy with strscpy)

[Johannes Berg]

On Tue, 2023-06-06 at 21:23 -0700, Eric Biggers wrote:
> 
> > > > Reported-by: kernel test robot <lkp@intel.com>
> > > > Closes:
> > > > https://lore.kernel.org/oe-kbuild-all/202305311135.zGMT1gYR-lkp@intel.com/
> > > 
> > > Are you sure Reported-by and Closes make sense?
> > > AFAIK the report was only on your first patch and nothing against upstream.
> > > So stating this in the updated patch is in vain.
> > 
> > I left the metadata in only for the sake of posterity. If it's not
> > helpful, I'm ok with removing it.
> > 
> 
> IMO using Reported-by in cases like this is harmful, as it makes commits seem
> like bug fixes when they are not.

I've yet to see anyone disagree with that, and yet, the robot actively
asks for such tags to be included in patch resubmissions.

On the one hand, I can understand their desire to be recognised for
their efforts. At this point then we might suggest that we introduce a
different tag, say "Improved-by:" or "Issues-found-by:" or something.

On the other hand, I don't feel like we should give a robot more
recognition than we give _people_ reviewing, and we currently really
only recognise them by a Reviewed-by tag. Then again, that doesn't work
with the robot - it is pretty much looking at each patch and only
comments on a small fraction. We also really don't want it to comment on
each and every patch ...


So it seems we should ask the robot maintainers to just stop suggesting
those tags?

johannes

Re: Reported-by/Closes tag for uncommitted issues (was: Re: [PATCH v2] uml: Replace strlcpy with strscpy)

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "Johannes Berg" <johannes@sipsolutions.net>
> On Tue, 2023-06-06 at 21:23 -0700, Eric Biggers wrote:
>> 
>> > > > Reported-by: kernel test robot <lkp@intel.com>
>> > > > Closes:
>> > > > https://lore.kernel.org/oe-kbuild-all/202305311135.zGMT1gYR-lkp@intel.com/
>> > > 
>> > > Are you sure Reported-by and Closes make sense?
>> > > AFAIK the report was only on your first patch and nothing against upstream.
>> > > So stating this in the updated patch is in vain.
>> > 
>> > I left the metadata in only for the sake of posterity. If it's not
>> > helpful, I'm ok with removing it.
>> > 
>> 
>> IMO using Reported-by in cases like this is harmful, as it makes commits seem
>> like bug fixes when they are not.
> 
> I've yet to see anyone disagree with that, and yet, the robot actively
> asks for such tags to be included in patch resubmissions.
> 
> On the one hand, I can understand their desire to be recognised for
> their efforts. At this point then we might suggest that we introduce a
> different tag, say "Improved-by:" or "Issues-found-by:" or something.

Robots don't have feelings. No need to worry. ;-)
 
> On the other hand, I don't feel like we should give a robot more
> recognition than we give _people_ reviewing, and we currently really
> only recognise them by a Reviewed-by tag. Then again, that doesn't work
> with the robot - it is pretty much looking at each patch and only
> comments on a small fraction. We also really don't want it to comment on
> each and every patch ...
> 
> 
> So it seems we should ask the robot maintainers to just stop suggesting
> those tags?

Agreed.

Thanks,
//richard

Re: Reported-by/Closes tag for uncommitted issues (was: Re: [PATCH v2] uml: Replace strlcpy with strscpy)

[Philip Li]

On Wed, Jun 07, 2023 at 10:34:55AM +0200, Richard Weinberger wrote:
> ----- Ursprüngliche Mail -----
> > Von: "Johannes Berg" <johannes@sipsolutions.net>
> > On Tue, 2023-06-06 at 21:23 -0700, Eric Biggers wrote:
> >> 
> >> > > > Reported-by: kernel test robot <lkp@intel.com>
> >> > > > Closes:
> >> > > > https://lore.kernel.org/oe-kbuild-all/202305311135.zGMT1gYR-lkp@intel.com/
> >> > > 
> >> > > Are you sure Reported-by and Closes make sense?
> >> > > AFAIK the report was only on your first patch and nothing against upstream.
> >> > > So stating this in the updated patch is in vain.
> >> > 
> >> > I left the metadata in only for the sake of posterity. If it's not
> >> > helpful, I'm ok with removing it.
> >> > 
> >> 
> >> IMO using Reported-by in cases like this is harmful, as it makes commits seem
> >> like bug fixes when they are not.
> > 
> > I've yet to see anyone disagree with that, and yet, the robot actively
> > asks for such tags to be included in patch resubmissions.
> > 
> > On the one hand, I can understand their desire to be recognised for
> > their efforts. At this point then we might suggest that we introduce a
> > different tag, say "Improved-by:" or "Issues-found-by:" or something.
> 
> Robots don't have feelings. No need to worry. ;-)
>  
> > On the other hand, I don't feel like we should give a robot more
> > recognition than we give _people_ reviewing, and we currently really
> > only recognise them by a Reviewed-by tag. Then again, that doesn't work
> > with the robot - it is pretty much looking at each patch and only
> > comments on a small fraction. We also really don't want it to comment on
> > each and every patch ...
> > 
> > 
> > So it seems we should ask the robot maintainers to just stop suggesting
> > those tags?
> 
> Agreed.

Thanks all for the feedback. We will carefully consider how to present the
suggestion clearly.

For now, because the bot covers both upstream and developer repos, there
can be various situations, such as the bug is found in upstream. So the bot
tries to let author decide how to apply the tags in appropriate way that
they feel comfortable.

In the report, we now uses phrases like below

	If you fix the issue, kindly add following tag where applicable
	| Reported-by: kernel test robot <lkp@intel.com>
	| Closes: https://lore.kernel.org/oe-kbuild-all/202305311135.zGMT1gYR-lkp@intel.com/

But this may be not clear enough or not the best way to suggest. We will
consider whether we can detect some situations (like RFC patch) which is
no need for such tags to avoid confusion.

> 
> Thanks,
> //richard

Re: Reported-by/Closes tag for uncommitted issues (was: Re: [PATCH v2] uml: Replace strlcpy with strscpy)

[Johannes Berg]

On Wed, 2023-06-07 at 17:10 +0800, Philip Li wrote:
> > > So it seems we should ask the robot maintainers to just stop suggesting
> > > those tags?
> > 
> > Agreed.
> 
> Thanks all for the feedback. We will carefully consider how to present the
> suggestion clearly.
> 
> For now, because the bot covers both upstream and developer repos, there
> can be various situations, such as the bug is found in upstream. 

Ah yes, that was actually in my mind, but I forgot to write about it,
sorry.

I agree completely, in case that you find a bug in an already committed
tree, and there will be a separate commit to fix it, it's completely
reasonable and useful to have those tags.

> So the bot
> tries to let author decide how to apply the tags in appropriate way that
> they feel comfortable.

Right. It just seems that many authors aren't really all that familiar
with the processes yet, and take the suggestion at face value.

> In the report, we now uses phrases like below
> 
> 	If you fix the issue, kindly add following tag where applicable
> 	| Reported-by: kernel test robot <lkp@intel.com>
> 	| Closes: https://lore.kernel.org/oe-kbuild-all/202305311135.zGMT1gYR-lkp@intel.com/
> 
> But this may be not clear enough or not the best way to suggest. We will
> consider whether we can detect some situations (like RFC patch) which is
> no need for such tags to avoid confusion.
> 

Right. Maybe the only thing really needed would be to say something like

"If you fix the issue in a separate patch/commit (i.e. not just a new
version of the same patch/commit), kindly add ..."

or even just

"If you fix the issue in a separate commit, kindly add ..."

so it's clear that if you're changing the commit, it's not really
something that should be done? In which case probably even a Fixes tag
should be there, but I wouldn't want to recommend adding that since the
commits may still change etc.

I don't know all the processes behind it, but I'm thinking that even if
the bot picked up a patch from the list, it could get committed before
and then fixed in a separate commit.

johannes

Re: Reported-by/Closes tag for uncommitted issues (was: Re: [PATCH v2] uml: Replace strlcpy with strscpy)

[Philip Li]

On Wed, Jun 07, 2023 at 11:17:54AM +0200, Johannes Berg wrote:
> On Wed, 2023-06-07 at 17:10 +0800, Philip Li wrote:
> > > > So it seems we should ask the robot maintainers to just stop suggesting
> > > > those tags?
> > > 
> > > Agreed.
> > 
> > Thanks all for the feedback. We will carefully consider how to present the
> > suggestion clearly.
> > 
> > For now, because the bot covers both upstream and developer repos, there
> > can be various situations, such as the bug is found in upstream. 
> 
> Ah yes, that was actually in my mind, but I forgot to write about it,
> sorry.
> 
> I agree completely, in case that you find a bug in an already committed
> tree, and there will be a separate commit to fix it, it's completely
> reasonable and useful to have those tags.
> 
> > So the bot
> > tries to let author decide how to apply the tags in appropriate way that
> > they feel comfortable.
> 
> Right. It just seems that many authors aren't really all that familiar
> with the processes yet, and take the suggestion at face value.
> 
> > In the report, we now uses phrases like below
> > 
> > 	If you fix the issue, kindly add following tag where applicable
> > 	| Reported-by: kernel test robot <lkp@intel.com>
> > 	| Closes: https://lore.kernel.org/oe-kbuild-all/202305311135.zGMT1gYR-lkp@intel.com/
> > 
> > But this may be not clear enough or not the best way to suggest. We will
> > consider whether we can detect some situations (like RFC patch) which is
> > no need for such tags to avoid confusion.
> > 
> 
> Right. Maybe the only thing really needed would be to say something like

Thanks a lot, the suggestion really helps us to get this better.

> 
> "If you fix the issue in a separate patch/commit (i.e. not just a new
> version of the same patch/commit), kindly add ..."

Is that ok we just take this phrase as a quick improvement for first step, which
is

"If you fix the issue in a separate patch/commit (i.e. not just a new
version of the same patch/commit), kindly add following tags:"

This could help remind for most cases if not all. Also this allows us
not doing "complex" judgement by the bot itself.

> 
> or even just
> 
> "If you fix the issue in a separate commit, kindly add ..."
> 
> so it's clear that if you're changing the commit, it's not really
> something that should be done? In which case probably even a Fixes tag
> should be there, but I wouldn't want to recommend adding that since the
> commits may still change etc.
> 
> I don't know all the processes behind it, but I'm thinking that even if
> the bot picked up a patch from the list, it could get committed before
> and then fixed in a separate commit.

You are right, thanks for reminding this. The bot monitors both patches
in the mailing list and repos of developers. It could happen that a patch
exists in both place, though there's logic to avoid testing both but can't
promise which side got tested first.

> 
> johannes

Re: Reported-by/Closes tag for uncommitted issues (was: Re: [PATCH v2] uml: Replace strlcpy with strscpy)

[Johannes Berg]

On Wed, 2023-06-07 at 17:39 +0800, Philip Li wrote:
> 
> Is that ok we just take this phrase as a quick improvement for first step, which
> is
> 
> "If you fix the issue in a separate patch/commit (i.e. not just a new
> version of the same patch/commit), kindly add following tags:"
> 
> This could help remind for most cases if not all. Also this allows us
> not doing "complex" judgement by the bot itself.

Sure, feel free. But yeah, that was the idea, that the decision logic
wouldn't really need to be in the bot, that feels very difficult.

Maybe someone else on the thread will have a better suggestion :)


johannes

Re: Reported-by/Closes tag for uncommitted issues (was: Re: [PATCH v2] uml: Replace strlcpy with strscpy)

[Philip Li]

On Wed, Jun 07, 2023 at 11:43:11AM +0200, Johannes Berg wrote:
> On Wed, 2023-06-07 at 17:39 +0800, Philip Li wrote:
> > 
> > Is that ok we just take this phrase as a quick improvement for first step, which
> > is
> > 
> > "If you fix the issue in a separate patch/commit (i.e. not just a new
> > version of the same patch/commit), kindly add following tags:"
> > 
> > This could help remind for most cases if not all. Also this allows us
> > not doing "complex" judgement by the bot itself.
> 
> Sure, feel free. But yeah, that was the idea, that the decision logic
> wouldn't really need to be in the bot, that feels very difficult.
> 
> Maybe someone else on the thread will have a better suggestion :)

Thanks a lot, we will adjust the phrase like this as initial step to resolve
some confusions. And as you said, we welcome more suggestions to improve
the bot.

> 
> 
> johannes