diff mbox series

net: wwan: replace deprecated strncpy with strscpy_pad

Message ID 20231018-strncpy-drivers-net-wwan-rpmsg_wwan_ctrl-c-v1-1-4e343270373a@google.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series net: wwan: replace deprecated strncpy with strscpy_pad | expand

Checks

Context Check Description
netdev/series_format warning Single patches do not need cover letters; Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1363 this patch: 1363
netdev/cc_maintainers success CCed 10 of 10 maintainers
netdev/build_clang success Errors and warnings before: 1388 this patch: 1388
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1388 this patch: 1388
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 8 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Justin Stitt Oct. 18, 2023, 10:14 p.m. UTC 
strncpy() is deprecated for use on NUL-terminated destination strings
[1] and as such we should prefer more robust and less ambiguous string
interfaces.

We expect chinfo.name to be NUL-terminated based on its use with format
strings and sprintf:
rpmsg/rpmsg_char.c
165:            dev_err(dev, "failed to open %s\n", eptdev->chinfo.name);
368:    return sprintf(buf, "%s\n", eptdev->chinfo.name);

... and with strcmp():
|  static struct rpmsg_endpoint *qcom_glink_create_ept(struct rpmsg_device *rpdev,
|  						    rpmsg_rx_cb_t cb,
|  						    void *priv,
|  						    struct rpmsg_channel_info
|  									chinfo)
|  ...
|  const char *name = chinfo.name;
|  ...
|  		if (!strcmp(channel->name, name))

Moreover, as chinfo is not kzalloc'd, let's opt to NUL-pad the
destination buffer

Similar change to:
Commit 766279a8f85d ("rpmsg: qcom: glink: replace strncpy() with strscpy_pad()")
and
Commit 08de420a8014 ("rpmsg: glink: Replace strncpy() with strscpy_pad()")

Considering the above, a suitable replacement is `strscpy_pad` due to
the fact that it guarantees both NUL-termination and NUL-padding on the
destination buffer.

Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
Note: build-tested only.

Found with: $ rg "strncpy\("
---
 drivers/net/wwan/rpmsg_wwan_ctrl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


---
base-commit: 58720809f52779dc0f08e53e54b014209d13eebb
change-id: 20231018-strncpy-drivers-net-wwan-rpmsg_wwan_ctrl-c-3f620aafd326

Best regards,
--
Justin Stitt <justinstitt@google.com>

Comments

Kees Cook Oct. 19, 2023, 5:35 a.m. UTC | #1 
On Wed, Oct 18, 2023 at 10:14:55PM +0000, Justin Stitt wrote:
> strncpy() is deprecated for use on NUL-terminated destination strings
> [1] and as such we should prefer more robust and less ambiguous string
> interfaces.
> 
> We expect chinfo.name to be NUL-terminated based on its use with format
> strings and sprintf:
> rpmsg/rpmsg_char.c
> 165:            dev_err(dev, "failed to open %s\n", eptdev->chinfo.name);
> 368:    return sprintf(buf, "%s\n", eptdev->chinfo.name);
> 
> ... and with strcmp():
> |  static struct rpmsg_endpoint *qcom_glink_create_ept(struct rpmsg_device *rpdev,
> |  						    rpmsg_rx_cb_t cb,
> |  						    void *priv,
> |  						    struct rpmsg_channel_info
> |  									chinfo)
> |  ...
> |  const char *name = chinfo.name;
> |  ...
> |  		if (!strcmp(channel->name, name))
> 
> Moreover, as chinfo is not kzalloc'd, let's opt to NUL-pad the
> destination buffer
> 
> Similar change to:
> Commit 766279a8f85d ("rpmsg: qcom: glink: replace strncpy() with strscpy_pad()")
> and
> Commit 08de420a8014 ("rpmsg: glink: Replace strncpy() with strscpy_pad()")
> 
> Considering the above, a suitable replacement is `strscpy_pad` due to
> the fact that it guarantees both NUL-termination and NUL-padding on the
> destination buffer.
> 
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> ---
> Note: build-tested only.
> 
> Found with: $ rg "strncpy\("
> ---
>  drivers/net/wwan/rpmsg_wwan_ctrl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wwan/rpmsg_wwan_ctrl.c b/drivers/net/wwan/rpmsg_wwan_ctrl.c
> index 86b60aadfa11..39f5e780c478 100644
> --- a/drivers/net/wwan/rpmsg_wwan_ctrl.c
> +++ b/drivers/net/wwan/rpmsg_wwan_ctrl.c
> @@ -37,7 +37,7 @@ static int rpmsg_wwan_ctrl_start(struct wwan_port *port)
>  		.dst = RPMSG_ADDR_ANY,
>  	};

"chinfo" is initialized immediately above here, which means that it is
actually already zero filled for all the members that aren't explicitly
initialized, so the _pad variant isn't needed. I suspect Dead Store
Elimination will optimize it all away anyway, so this is probably fine.

>  
> -	strncpy(chinfo.name, rpwwan->rpdev->id.name, RPMSG_NAME_SIZE);
> +	strscpy_pad(chinfo.name, rpwwan->rpdev->id.name, sizeof(chinfo.name));

Yup, sizeof() replacement looks correct:

struct rpmsg_channel_info {
        char name[RPMSG_NAME_SIZE];

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees
Stephan Gerhold Oct. 19, 2023, 1:39 p.m. UTC | #2 
On Wed, Oct 18, 2023 at 10:35:26PM -0700, Kees Cook wrote:
> On Wed, Oct 18, 2023 at 10:14:55PM +0000, Justin Stitt wrote:
> > strncpy() is deprecated for use on NUL-terminated destination strings
> > [1] and as such we should prefer more robust and less ambiguous string
> > interfaces.
> > 
> > We expect chinfo.name to be NUL-terminated based on its use with format
> > strings and sprintf:
> > rpmsg/rpmsg_char.c
> > 165:            dev_err(dev, "failed to open %s\n", eptdev->chinfo.name);
> > 368:    return sprintf(buf, "%s\n", eptdev->chinfo.name);
> > 
> > ... and with strcmp():
> > |  static struct rpmsg_endpoint *qcom_glink_create_ept(struct rpmsg_device *rpdev,
> > |  						    rpmsg_rx_cb_t cb,
> > |  						    void *priv,
> > |  						    struct rpmsg_channel_info
> > |  									chinfo)
> > |  ...
> > |  const char *name = chinfo.name;
> > |  ...
> > |  		if (!strcmp(channel->name, name))
> > 
> > Moreover, as chinfo is not kzalloc'd, let's opt to NUL-pad the
> > destination buffer
> > 
> > Similar change to:
> > Commit 766279a8f85d ("rpmsg: qcom: glink: replace strncpy() with strscpy_pad()")
> > and
> > Commit 08de420a8014 ("rpmsg: glink: Replace strncpy() with strscpy_pad()")
> > 
> > Considering the above, a suitable replacement is `strscpy_pad` due to
> > the fact that it guarantees both NUL-termination and NUL-padding on the
> > destination buffer.
> > 
> > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> > Link: https://github.com/KSPP/linux/issues/90
> > Cc: linux-hardening@vger.kernel.org
> > Signed-off-by: Justin Stitt <justinstitt@google.com>
> > ---
> > Note: build-tested only.
> > 
> > Found with: $ rg "strncpy\("
> > ---
> >  drivers/net/wwan/rpmsg_wwan_ctrl.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/wwan/rpmsg_wwan_ctrl.c b/drivers/net/wwan/rpmsg_wwan_ctrl.c
> > index 86b60aadfa11..39f5e780c478 100644
> > --- a/drivers/net/wwan/rpmsg_wwan_ctrl.c
> > +++ b/drivers/net/wwan/rpmsg_wwan_ctrl.c
> > @@ -37,7 +37,7 @@ static int rpmsg_wwan_ctrl_start(struct wwan_port *port)
> >  		.dst = RPMSG_ADDR_ANY,
> >  	};
> 
> "chinfo" is initialized immediately above here, which means that it is
> actually already zero filled for all the members that aren't explicitly
> initialized, so the _pad variant isn't needed. I suspect Dead Store
> Elimination will optimize it all away anyway, so this is probably fine.
> 

Hm, strscpy_pad() is neither a typical compiler builtin nor an inline
function, so my naive assumption would be that this could only be
optimized away with LTO?

But I don't think this is particularly performance critical code, so
maybe it's even better to be explicit in case someone ever changes the
way chinfo is allocated.

@Justin: Nevertheless I would appreciate if you could briefly reword the
commit message and add a note about this. Someone reading it later might
get confused or mislead by the "Moreover, as chinfo is not kzalloc'd,"
part. As Kees wrote, even without kzalloc the struct initializer of
chinfo does actually ensure proper zero initialization of the missing
members.

Thanks!
Stephan
Kees Cook Oct. 20, 2023, 12:15 a.m. UTC | #3 
On Thu, Oct 19, 2023 at 03:39:10PM +0200, Stephan Gerhold wrote:
> Hm, strscpy_pad() is neither a typical compiler builtin nor an inline
> function, so my naive assumption would be that this could only be
> optimized away with LTO?

Oops, yes, my mistake. I'm too used to the other fortified helpers that
are inlined...
diff mbox series

Patch

diff --git a/drivers/net/wwan/rpmsg_wwan_ctrl.c b/drivers/net/wwan/rpmsg_wwan_ctrl.c
index 86b60aadfa11..39f5e780c478 100644
--- a/drivers/net/wwan/rpmsg_wwan_ctrl.c
+++ b/drivers/net/wwan/rpmsg_wwan_ctrl.c
@@ -37,7 +37,7 @@  static int rpmsg_wwan_ctrl_start(struct wwan_port *port)
 		.dst = RPMSG_ADDR_ANY,
 	};
 
-	strncpy(chinfo.name, rpwwan->rpdev->id.name, RPMSG_NAME_SIZE);
+	strscpy_pad(chinfo.name, rpwwan->rpdev->id.name, sizeof(chinfo.name));
 	rpwwan->ept = rpmsg_create_ept(rpwwan->rpdev, rpmsg_wwan_ctrl_callback,
 				       rpwwan, chinfo);
 	if (!rpwwan->ept)