RE: [PATCH net-next v3] tipc: replace deprecated strcpy with strscpy in tipc_bearer_get_name()

Tung Quang Nguyen <[email protected]>
Newsgroups gmane.linux.network,gmane.network.tipc.general,gmane.linux.kernel
Message-ID <GV1P189MB1988B4FA71085BFFB31AF4CDC6A72@GV1P189MB1988.EURP189.PROD.OUTLOOK.COM>
>Subject: Re: [PATCH net-next v3] tipc: replace deprecated strcpy with strscpy in
>tipc_bearer_get_name()
>
>> net tree:
>> https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git
>> net-next tree:
>> https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git
>
>Thank you. I tried the correct net-next tree. The same patch applied
>successfully here also as shown below.
>
><path of the linux tree>/net-next$ ls
>arch   certs    CREDITS  Documentation  fs       init      ipc     Kconfig  lib
>MAINTAINERS  mm   README  samples  security  tools  virt
>block  COPYING  crypto   drivers        include  io_uring  Kbuild  kernel   LICENSES
>Makefile     net  rust    scripts  sound     usr
><path of the linux tree>/net-next$ git remote -v
>origin	https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git
>(fetch)
>origin	https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git
>(push)
><path of the linux tree>/net-next$ git am ../../patches/v3-0001-tipc-replace-
>deprecated-strcpy-with-strscpy-in-ti.patch
>Applying: tipc: replace deprecated strcpy with strscpy in
>tipc_bearer_get_name() <path of the linux tree>/net-next$ git branch
>* main
><path of the linux tree>/net-next$
>
>Am I missing something?

I see that your patch was applied again successfully after failure.
There is one feedback from AI review that I see valid:
[...]
> @@ -209,7 +209,7 @@ int tipc_bearer_get_name(struct net *net, char *name, u32 bearer_id)
>  	if (!b)
>  		return -EINVAL;
>  
> -	strcpy(name, b->name);
> +	strscpy(name, b->name, TIPC_MAX_BEARER_NAME);
This isn't a bug, but the destination capacity is now hard-coded in the
body while the interface says nothing about it.  The declaration in
net/tipc/bearer.h is still:
	int tipc_bearer_get_name(struct net *net, char *name, u32 bearer_id);
and the comment directly above the implementation only says:
	 *     @name: a pointer to the buffer where the name will be stored.
Should the TIPC_MAX_BEARER_NAME requirement be spelled out in the comment
or in the prototype, so that the interface and the body agree?
In the current tree there is no behavioural change: the only caller is
__tipc_nl_add_monitor() in net/tipc/monitor.c, which declares
char bearer_name[TIPC_MAX_BEARER_NAME], and the source is
char name[TIPC_MAX_BEARER_NAME] in struct tipc_bearer, so the copy can
neither truncate nor overflow.
The strscpy() return value is also discarded, so a future caller passing a
smaller buffer would get a silently truncated name plus a return of 0.
Would passing a size_t len from the caller (sizeof(bearer_name)) and
forwarding strscpy()'s -E2BIG through the existing int return be preferable
to a hard-coded bound?
[...]

Please update your patch and send the new version after Aug 31st. (Because net-next is closed now)
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.