| Message ID | 20231018-strncpy-drivers-net-wwan-rpmsg_wwan_ctrl-c-v1-1-4e343270373a@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | net: wwan: replace deprecated strncpy with strscpy_pad | expand |
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
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
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 --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)
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>