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: [email protected]
Signed-off-by: Justin Stitt <[email protected]>
---
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,
};
- 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)
---
base-commit: 58720809f52779dc0f08e53e54b014209d13eebb
change-id: 20231018-strncpy-drivers-net-wwan-rpmsg_wwan_ctrl-c-3f620aafd326
Best regards,
--
Justin Stitt <[email protected]>
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: [email protected]
> Signed-off-by: Justin Stitt <[email protected]>
> ---
> 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 <[email protected]>
-Kees
--
Kees Cook
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: [email protected]
> > Signed-off-by: Justin Stitt <[email protected]>
> > ---
> > 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...
--
Kees Cook