Message ID | 20231018-strncpy-drivers-net-wwan-rpmsg_wwan_ctrl-c-v1-1-4e343270373a@google.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:612c:2010:b0:403:3b70:6f57 with SMTP id fe16csp26291vqb; Wed, 18 Oct 2023 15:15:37 -0700 (PDT) X-Google-Smtp-Source: AGHT+IGMHZnmZs1AqjjV8t6MCszdt1j65/vjJjOG3RoS1+CHGarfupaqqZaje9+MtmNRNEkSdA7N X-Received: by 2002:a05:6a20:8f26:b0:15e:b763:2422 with SMTP id b38-20020a056a208f2600b0015eb7632422mr261449pzk.9.1697667337255; Wed, 18 Oct 2023 15:15:37 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1697667337; cv=none; d=google.com; s=arc-20160816; b=RpkReH1XNix7mtGPQFwkux6/w5d+p8rm9Y7hoii3v/COP9sKYx0iyYHjC1l/xk/L+u VYG59Xe58eIWC5OHYpsRdKEorVdPoiJiwWyUf6RpNOLg3P5IAAOeUjcIEz/uvrfDAOv0 cUNlki63yfZCviujMgoEhuDwa1l0i/Hhtz86tTQBDDoUKydCoZ63WJr97RWGNsPP+QzG 5jGvcqJ6CARA8m6hyeJdyto37vYRSM6A4WOoW/snm2g359hIkjDxFDix53jbCdEjseaE DkvQhGv2fM7BXeDlkEegIvZ/0Cu4POhBBWvsffMF+6mm7j+zZ6A1e78MiDXte0v7tj9r /5CA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:from:subject:message-id:mime-version:date :dkim-signature; bh=PlQsMleo8ndyb1BkvS8psG/NpipwSb1Ibvd6wvaptgE=; fh=yE1HzpQBjlbARPvY9MVc+QRwZUL/7jbR+OSvlBpDge4=; b=f89TKdZUCF2vihcLUAp+3Cho0RaecSWvzpDC6MAO0R+qWwO5mHC9BLHStxdC6/0Pjw GifU64tnEzeuGgPlqwXNY5DeBBPatWG8Q9ikRtZNLGqdpGdMNE2vFJtmZuU8MWlFdZzB bztIXKGSEinwmP+YwZ8bnS/uKI30KLuTU/RTR9v0q42mseIx+BlSbfHHb6jYwTX3B3ei Wk8gEgEJiFZCZFCXz9P+8zxRaV1iqfz4KqUPJ3+nFg+1zQez/2WMipgfTyr8a0SbnOSD Z/SxNGH4a/HolioZQ8ph6DgfY+TLGHdhl86j2ji1lXkgU1P4/CXbZxpibB/HduY+9bVJ HbQA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20230601 header.b=rpD9nXDr; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: from agentk.vger.email (agentk.vger.email. [23.128.96.32]) by mx.google.com with ESMTPS id t4-20020a63f344000000b005789c7e9bf2si3049166pgj.627.2023.10.18.15.15.36 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 18 Oct 2023 15:15:37 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32; Authentication-Results: mx.google.com; dkim=pass header.i=@google.com header.s=20230601 header.b=rpD9nXDr; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by agentk.vger.email (Postfix) with ESMTP id DE2BA8090274; Wed, 18 Oct 2023 15:15:17 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at agentk.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230391AbjJRWO7 (ORCPT <rfc822;zwp10758@gmail.com> + 24 others); Wed, 18 Oct 2023 18:14:59 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53558 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229924AbjJRWO6 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 18 Oct 2023 18:14:58 -0400 Received: from mail-yw1-x114a.google.com (mail-yw1-x114a.google.com [IPv6:2607:f8b0:4864:20::114a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A931C115 for <linux-kernel@vger.kernel.org>; Wed, 18 Oct 2023 15:14:56 -0700 (PDT) Received: by mail-yw1-x114a.google.com with SMTP id 00721157ae682-5a7ed6903a6so111710727b3.2 for <linux-kernel@vger.kernel.org>; Wed, 18 Oct 2023 15:14:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1697667296; x=1698272096; darn=vger.kernel.org; h=cc:to:from:subject:message-id:mime-version:date:from:to:cc:subject :date:message-id:reply-to; bh=PlQsMleo8ndyb1BkvS8psG/NpipwSb1Ibvd6wvaptgE=; b=rpD9nXDr8ZV4X1Zjrx1Lpkbi8PcVhM+FAsPQEkE/bgGqvJdPH1dZ/7vggwFyV7nhEA X8bQ+c4ABe5Lv0Lx4UdVxd3dONLa3xGFTYGjnsytjjaPuWHDgsSwaDQqOqxNYmUHL3E8 fd6cI8mvnnYu0l+EvsSOxOG18UhCTsNL6jB/fhq9pjPCpnyyxwUQHxSZ87gnNTZIS6r+ iRqmgKZXyA5v+lImVMteIruljC+BI0JQ11H/uZKwoOFulZPDL9CV4UpMWYejLAO+vAfZ /dHi5bk0o61IOex9EqNmqWyAwgpM35REgb1kPuw72D9Xa/TL/git7S9ZvXE7Ceui1EnK 6sYA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1697667296; x=1698272096; h=cc:to:from:subject:message-id:mime-version:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=PlQsMleo8ndyb1BkvS8psG/NpipwSb1Ibvd6wvaptgE=; b=nq9CnByXr/798ODj/VQlAzdXLHO5zqK9joSo8R6PGS6PY9edQSAidNnEt4grslwe6v ffuSilYDpzAArqj5NmaXICVTOTNvCDCO6O1PbZ7grPnQbJdGR7qxUUQSBBPfZqlxIqfl +KKli2IxKHrH0gvkyGKjEO3Nw2n1aFGE8mFJalpLeXHcdnokMYM1+t715hzLjI66r/1L Hu0qe8/kLe/nKFA18PipOUyr2ASODZ5qyyYh3oenAAoujpqGCItv8+Y3VoLi0svX8y+d 7cAbXnlshvGd7IagDqbIK/2v0zEaMoObCtf2Uf4ddOdjU1eiZ3D6CXi1fSl11+et9uau Psmw== X-Gm-Message-State: AOJu0YwKYck43ERUfRtKzo7bJzGRtwhv+RI+ZitXH3Qq+yq1aE9PJ05+ WDQjTpeNfsi8dpRmg6R8kpw0YphhRovYtHrKbA== X-Received: from jstitt-linux1.c.googlers.com ([fda3:e722:ac3:cc00:2b:ff92:c0a8:23b5]) (user=justinstitt job=sendgmr) by 2002:a0d:d9d2:0:b0:5a7:be10:461d with SMTP id b201-20020a0dd9d2000000b005a7be10461dmr15636ywe.2.1697667295839; Wed, 18 Oct 2023 15:14:55 -0700 (PDT) Date: Wed, 18 Oct 2023 22:14:55 +0000 Mime-Version: 1.0 X-B4-Tracking: v=1; b=H4sIAN5YMGUC/x2NywrCMBAAf6Xs2YU8oIi/IlJCsqkLGsNuaJXSf 2/qbeYys4GSMCnchg2EFlb+lC72MkB8hjITcuoOzjhvjb2iNimx/jAJLySKhRquaygo9a3zdOI Um7wwos+jMyHk5N0IPViFMn//s/tj3w8hAG6ffAAAAA== X-Developer-Key: i=justinstitt@google.com; a=ed25519; pk=tC3hNkJQTpNX/gLKxTNQKDmiQl6QjBNCGKJINqAdJsE= X-Developer-Signature: v=1; a=ed25519-sha256; t=1697667294; l=2364; i=justinstitt@google.com; s=20230717; h=from:subject:message-id; bh=+RiFAuTFDYeSx5+FsqkeyVgCujTXIxMDn3P2javi3XY=; b=123OWlCw+sopcVGOlRvo8H8eZ3pupMbwmZbCt7KUM82cnX6fdX3BK+EJUTP0xC4c1b3gbO3IB 1w6u2cozf6AB/MUjQvfMIuLfOn9l4T+IXmXkdkHqvfMcPANOo65c801 X-Mailer: b4 0.12.3 Message-ID: <20231018-strncpy-drivers-net-wwan-rpmsg_wwan_ctrl-c-v1-1-4e343270373a@google.com> Subject: [PATCH] net: wwan: replace deprecated strncpy with strscpy_pad From: Justin Stitt <justinstitt@google.com> To: Stephan Gerhold <stephan@gerhold.net>, 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> Cc: netdev@vger.kernel.org, linux-remoteproc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org, Justin Stitt <justinstitt@google.com> Content-Type: text/plain; charset="utf-8" X-Spam-Status: No, score=-8.4 required=5.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Wed, 18 Oct 2023 15:15:17 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1780133226069839920 X-GMAIL-MSGID: 1780133226069839920 |
Series |
net: wwan: replace deprecated strncpy with strscpy_pad
|
|
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
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)