Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Date:   Thu, 19 Oct 2023 15:39:10 +0200
From:   Stephan Gerhold <stephan@gerhold.net>
To:     Kees Cook <keescook@chromium.org>,
        Justin Stitt <justinstitt@google.com>
Cc:     Loic Poulain <loic.poulain@linaro.org>,
        Sergey Ryazanov <ryazanov.s.a@gmail.com>,
        Johannes Berg <johannes@sipsolutions.net>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, netdev@vger.kernel.org,
        linux-remoteproc@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-hardening@vger.kernel.org
Subject: Re: [PATCH] net: wwan: replace deprecated strncpy with strscpy_pad
Message-ID: <ZTExfv2aHPD2B1ze@gerhold.net>
References: <20231018-strncpy-drivers-net-wwan-rpmsg_wwan_ctrl-c-v1-1-4e343270373a@google.com>
 <202310182232.A569D262@keescook>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <202310182232.A569D262@keescook>
Content-Transfer-Encoding: 7bit
Precedence: bulk

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