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)