Message ID | 20231026-strncpy-drivers-net-wireless-cisco-airo-c-v2-1-413427249e47@google.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:d641:0:b0:403:3b70:6f57 with SMTP id cy1csp232501vqb; Thu, 26 Oct 2023 16:19:54 -0700 (PDT) X-Google-Smtp-Source: AGHT+IGOp5/M4i4D+vs6vq6RCtXxy+jSg9mh4WgMyEdSQvJe2SAjSSrWt8MZ3WFVvSdgDqw+M1d6 X-Received: by 2002:a25:5097:0:b0:d9a:5aa0:7a30 with SMTP id e145-20020a255097000000b00d9a5aa07a30mr831089ybb.39.1698362393795; Thu, 26 Oct 2023 16:19:53 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1698362393; cv=none; d=google.com; s=arc-20160816; b=DZMQI9oPxrH/I4tTtWz9/lG/SulmxVFsgSSKIBa1YBT3fYVfTmlS401HNaQ3fzg8NM qVn/+HXTON3Y4AvarRuhdoXrJ1I0n8H+tkI5wkLCTEFGZYOpxgme+ZfWS3oIfErWGF8a Z2UFbsAvavw3NiUpfa5SVkI6k2y2r6mOtmzrBEE5gzumtcqAMLaCnCRq0OMs37tiQovN GKhw8klpF4/RrYPYc5WLKI6cShctLEJz/9tVQlymyVOcTgFxhKC5wQmVexUfB3vJFKJT XxUyDyHNgCjoebdEOUtVMKxz7IrvqvI8+BmoWV/rn+QKnLdygzyQiBudsjszH5P7bou6 HFLw== 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=rJUIaaq/oswhFA94o3ND9d9+Iexffy2MpltZ9TbSQQY=; fh=JWAO1fAe+Wf5O2DamUx86WOaLpB0s7CTfUExwjP/ICk=; b=T/O3zJHh0Kve6ix7BGU6JMODu0rEuA0IUr18GSrBa+EuAaeKBdeZjtbmruxWt7ClG+ bVLG12IQNvcbz9rpLj8sCCXFcEhWYmCgY9MShtnMg8xQg3vg3vNKzibQvHZkfv2sI8rR ag3ap+9heKxspdM4QCd+PMjpN2ub0kwr6xG2iDzY+hqNl2jLER7frPrcnKANo4t2hEOk pE2wNrsA4dpjdMs2h23PP0gmLsnIrETuWpvDHYgJZvxA//48gvi9jFh3JHOzJQQQnq+k GiR9NHDasGvIiIlmfkrGten/3WhbiikLFe/1z0PCKyIjH14h+pQDCqHuobNv0t8xxSgC nIVQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20230601 header.b=BC6d5eOU; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 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 fry.vger.email (fry.vger.email. [2620:137:e000::3:8]) by mx.google.com with ESMTPS id a70-20020a25ca49000000b00d867e0791fcsi787920ybg.402.2023.10.26.16.19.53 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 26 Oct 2023 16:19:53 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8; Authentication-Results: mx.google.com; dkim=pass header.i=@google.com header.s=20230601 header.b=BC6d5eOU; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 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 fry.vger.email (Postfix) with ESMTP id 0A20582DEE6F; Thu, 26 Oct 2023 16:19:51 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at fry.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1345025AbjJZXTY (ORCPT <rfc822;aposhian.dev@gmail.com> + 26 others); Thu, 26 Oct 2023 19:19:24 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53394 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1344896AbjJZXTW (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 26 Oct 2023 19:19:22 -0400 Received: from mail-oo1-xc49.google.com (mail-oo1-xc49.google.com [IPv6:2607:f8b0:4864:20::c49]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B974D1A7 for <linux-kernel@vger.kernel.org>; Thu, 26 Oct 2023 16:19:19 -0700 (PDT) Received: by mail-oo1-xc49.google.com with SMTP id 006d021491bc7-581debabf87so2346749eaf.1 for <linux-kernel@vger.kernel.org>; Thu, 26 Oct 2023 16:19:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1698362359; x=1698967159; darn=vger.kernel.org; h=cc:to:from:subject:message-id:mime-version:date:from:to:cc:subject :date:message-id:reply-to; bh=rJUIaaq/oswhFA94o3ND9d9+Iexffy2MpltZ9TbSQQY=; b=BC6d5eOUbpZj2EhbDkCtoVPkarvXcccoL04DzKW8pl/jhqx1Q520C4Urca1Sko4OKp bavNq0XNv/2izUivQLIGWWR6M3faFCIa7rZ9ZBRM8mi8+aEFeTs5eNZzFR9nCX3wdkjF uThlr63OwueeAPgi/UTizMQ3P2jeEdcjoCj1Xo3+na26rAwqxQGfEgC9UtLPv3sarxqq v8LoUR9ZKTSAtLv0dBohaGBlqWXihME/P2Y9Mfis07mDh3MfCe8OfXfq2Zk1bk9hrAvQ sYWc9AEJeXa16H3UgQvsmPj+ARr2tgzjj9sTZ1AB+P6NQPJxhUWYKAFxA+HRDppn/Px8 KzNw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1698362359; x=1698967159; h=cc:to:from:subject:message-id:mime-version:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=rJUIaaq/oswhFA94o3ND9d9+Iexffy2MpltZ9TbSQQY=; b=HEI55EVpL0SpUGxo1sVcaQOvkCUmrLJHxVrcJIwrrLhP9e4SMAgTD/5lWzhv0aVQrP Y1co4lztFGRYSah9zNoIxCzvKWYsafu/sFpna8LflLE49t9mGKJv/V9SMUXwsR7/8eMX Gx++0eDc/CdiYwDEQKVoKJ03decS5pVa55kDtnc2m++eiHD2sBlaq+G5ZiutDDGL0Cry VVd95BQ4yZg9teEAenbBAEljOfi2z3j8byHjqswyVoY8WDsayruw64c7dbMmTglIlbtW vHGe619eZRasM+QjScLXkmsUrC6PBFEw0JyVThJL61p4YXWtVD0IFn932OJOD+JJIZp1 6rCQ== X-Gm-Message-State: AOJu0YwpG5kX28/+ZvaYxsSymvVDT8Tyh+kIe18u4J4qg5jxFP+rx13l 2LnqS8ViZNtru9c6xCFAjl0ehiggO7DCt2kuiQ== X-Received: from jstitt-linux1.c.googlers.com ([fda3:e722:ac3:cc00:2b:ff92:c0a8:23b5]) (user=justinstitt job=sendgmr) by 2002:a4a:e658:0:b0:581:dbb1:219b with SMTP id q24-20020a4ae658000000b00581dbb1219bmr404827oot.0.1698362359119; Thu, 26 Oct 2023 16:19:19 -0700 (PDT) Date: Thu, 26 Oct 2023 23:19:18 +0000 Mime-Version: 1.0 X-B4-Tracking: v=1; b=H4sIAPXzOmUC/53NQQ6CMBCF4auQrh3TUpHoinsYFtCOMAm2ZIZUC eHuVo7g8n+L921KkAlF3YtNMSYSiiFHeSqUG7swIJDPrUpdWqNNDbJwcPMKnikhCwRc4E2ME4q AI3EROuIIDry+Oa8rrbsrqvw3Mz7pc1iPNvdIskReDzqZ3/qPkgwYQHvxVW/7Gm3dDDEOE55df Kl23/cvKrGXxOMAAAA= X-Developer-Key: i=justinstitt@google.com; a=ed25519; pk=tC3hNkJQTpNX/gLKxTNQKDmiQl6QjBNCGKJINqAdJsE= X-Developer-Signature: v=1; a=ed25519-sha256; t=1698362358; l=2719; i=justinstitt@google.com; s=20230717; h=from:subject:message-id; bh=tC58vKAxLJm5cAdnZ9rYPmKDTa3guBZo7Oh8cfH/AXg=; b=xW/7pk2zSIdbdaANtOGtksyMjVeqD8M30IczBSFi365yP9f/D8GLqCR5vqlPF/2OcBvYQdmMr CAYKpl7GFTvD4/4vGd7LbAxPSijXKO8qWqRTUeMAOjbIl15/K3OHB/C X-Mailer: b4 0.12.3 Message-ID: <20231026-strncpy-drivers-net-wireless-cisco-airo-c-v2-1-413427249e47@google.com> Subject: [PATCH v2] airo: replace deprecated strncpy with strscpy_pad From: Justin Stitt <justinstitt@google.com> To: Kalle Valo <kvalo@kernel.org> Cc: linux-wireless@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 fry.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 (fry.vger.email [0.0.0.0]); Thu, 26 Oct 2023 16:19:51 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1780862045665363707 X-GMAIL-MSGID: 1780862045665363707 |
Series |
[v2] airo: replace deprecated strncpy with strscpy_pad
|
|
Commit Message
Justin Stitt
Oct. 26, 2023, 11:19 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.
`extra` is clearly supposed to be NUL-terminated which is evident by the
manual NUL-byte assignment as well as its immediate usage with strlen().
Moreover, let's NUL-pad since there is deliberate effort (48 instances)
made elsewhere to zero-out buffers in these getters and setters:
6050 | memset(local->config.nodeName, 0, sizeof(local->config.nodeName));
6130 | memset(local->config.rates, 0, 8);
6139 | memset(local->config.rates, 0, 8);
6414 | memset(key.key, 0, MAX_KEY_SIZE);
6497 | memset(extra, 0, 16);
(to be clear, strncpy also NUL-padded -- we are matching that behavior)
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.
We can also replace the hard-coded size of "16" to IW_ESSID_MAX_SIZE
because this function is a wext handler.
In wext-core.c we have:
static const struct iw_ioctl_description standard_ioctl[] = {
...
[IW_IOCTL_IDX(SIOCGIWNICKN)] = {
.header_type = IW_HEADER_TYPE_POINT,
.token_size = 1,
.max_tokens = IW_ESSID_MAX_SIZE,
},
So the buffer size is (strangely) IW_ESSID_MAX_SIZE
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>
---
Changes in v2:
- use IW_ESSID_MAX_SIZE (thanks Jeff, Kees)
- Link to v1: https://lore.kernel.org/r/20231017-strncpy-drivers-net-wireless-cisco-airo-c-v1-1-e34d5b3b7e37@google.com
---
Note: build-tested only.
Found with: $ rg "strncpy\("
---
drivers/net/wireless/cisco/airo.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
---
base-commit: 58720809f52779dc0f08e53e54b014209d13eebb
change-id: 20231017-strncpy-drivers-net-wireless-cisco-airo-c-d09cd0500a6e
Best regards,
--
Justin Stitt <justinstitt@google.com>
Comments
On 10/26/2023 4:19 PM, 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. > > `extra` is clearly supposed to be NUL-terminated which is evident by the > manual NUL-byte assignment as well as its immediate usage with strlen(). > > Moreover, let's NUL-pad since there is deliberate effort (48 instances) > made elsewhere to zero-out buffers in these getters and setters: > 6050 | memset(local->config.nodeName, 0, sizeof(local->config.nodeName)); > 6130 | memset(local->config.rates, 0, 8); > 6139 | memset(local->config.rates, 0, 8); > 6414 | memset(key.key, 0, MAX_KEY_SIZE); > 6497 | memset(extra, 0, 16); > (to be clear, strncpy also NUL-padded -- we are matching that behavior) > > 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. > > We can also replace the hard-coded size of "16" to IW_ESSID_MAX_SIZE > because this function is a wext handler. > > In wext-core.c we have: > static const struct iw_ioctl_description standard_ioctl[] = { > ... > [IW_IOCTL_IDX(SIOCGIWNICKN)] = { > .header_type = IW_HEADER_TYPE_POINT, > .token_size = 1, > .max_tokens = IW_ESSID_MAX_SIZE, > }, > > So the buffer size is (strangely) IW_ESSID_MAX_SIZE > > 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> Reviewed-by: Jeff Johnson <quic_jjohnson@quicinc.com>
On Thu, Oct 26, 2023 at 11:19:18PM +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. > > `extra` is clearly supposed to be NUL-terminated which is evident by the > manual NUL-byte assignment as well as its immediate usage with strlen(). > > Moreover, let's NUL-pad since there is deliberate effort (48 instances) > made elsewhere to zero-out buffers in these getters and setters: > 6050 | memset(local->config.nodeName, 0, sizeof(local->config.nodeName)); > 6130 | memset(local->config.rates, 0, 8); > 6139 | memset(local->config.rates, 0, 8); > 6414 | memset(key.key, 0, MAX_KEY_SIZE); > 6497 | memset(extra, 0, 16); > (to be clear, strncpy also NUL-padded -- we are matching that behavior) > > 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. > > We can also replace the hard-coded size of "16" to IW_ESSID_MAX_SIZE > because this function is a wext handler. > > In wext-core.c we have: > static const struct iw_ioctl_description standard_ioctl[] = { > ... > [IW_IOCTL_IDX(SIOCGIWNICKN)] = { > .header_type = IW_HEADER_TYPE_POINT, > .token_size = 1, > .max_tokens = IW_ESSID_MAX_SIZE, > }, > > So the buffer size is (strangely) IW_ESSID_MAX_SIZE > > 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> Looks good; thanks! Reviewed-by: Kees Cook <keescook@chromium.org>
Kees Cook <keescook@chromium.org> writes: > On Thu, Oct 26, 2023 at 11:19:18PM +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. >> >> `extra` is clearly supposed to be NUL-terminated which is evident by the >> manual NUL-byte assignment as well as its immediate usage with strlen(). >> >> Moreover, let's NUL-pad since there is deliberate effort (48 instances) >> made elsewhere to zero-out buffers in these getters and setters: >> 6050 | memset(local->config.nodeName, 0, sizeof(local->config.nodeName)); >> 6130 | memset(local->config.rates, 0, 8); >> 6139 | memset(local->config.rates, 0, 8); >> 6414 | memset(key.key, 0, MAX_KEY_SIZE); >> 6497 | memset(extra, 0, 16); >> (to be clear, strncpy also NUL-padded -- we are matching that behavior) >> >> 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. >> >> We can also replace the hard-coded size of "16" to IW_ESSID_MAX_SIZE >> because this function is a wext handler. >> >> In wext-core.c we have: >> static const struct iw_ioctl_description standard_ioctl[] = { >> ... >> [IW_IOCTL_IDX(SIOCGIWNICKN)] = { >> .header_type = IW_HEADER_TYPE_POINT, >> .token_size = 1, >> .max_tokens = IW_ESSID_MAX_SIZE, >> }, >> >> So the buffer size is (strangely) IW_ESSID_MAX_SIZE >> >> 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> > > Looks good; thanks! > > Reviewed-by: Kees Cook <keescook@chromium.org> BTW most likely next week this driver and a bunch of other ancient drivers will removed: https://patchwork.kernel.org/project/linux-wireless/list/?series=795639&state=*&order=date So to avoid unnecessary work on already removed drivers I recommend using wireless-next as the baseline for wireless patches. Though I'm still planning to apply this patch in case we ever add the driver back (I hope not).
Justin Stitt <justinstitt@google.com> 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. > > `extra` is clearly supposed to be NUL-terminated which is evident by the > manual NUL-byte assignment as well as its immediate usage with strlen(). > > Moreover, let's NUL-pad since there is deliberate effort (48 instances) > made elsewhere to zero-out buffers in these getters and setters: > 6050 | memset(local->config.nodeName, 0, sizeof(local->config.nodeName)); > 6130 | memset(local->config.rates, 0, 8); > 6139 | memset(local->config.rates, 0, 8); > 6414 | memset(key.key, 0, MAX_KEY_SIZE); > 6497 | memset(extra, 0, 16); > (to be clear, strncpy also NUL-padded -- we are matching that behavior) > > 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. > > We can also replace the hard-coded size of "16" to IW_ESSID_MAX_SIZE > because this function is a wext handler. > > In wext-core.c we have: > static const struct iw_ioctl_description standard_ioctl[] = { > ... > [IW_IOCTL_IDX(SIOCGIWNICKN)] = { > .header_type = IW_HEADER_TYPE_POINT, > .token_size = 1, > .max_tokens = IW_ESSID_MAX_SIZE, > }, > > So the buffer size is (strangely) IW_ESSID_MAX_SIZE > > 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> > Reviewed-by: Jeff Johnson <quic_jjohnson@quicinc.com> Patch applied to wireless-next.git, thanks. 9beac4ee4928 wifi: airo: replace deprecated strncpy with strscpy_pad
diff --git a/drivers/net/wireless/cisco/airo.c b/drivers/net/wireless/cisco/airo.c index dbd13f7aa3e6..6a099642e854 100644 --- a/drivers/net/wireless/cisco/airo.c +++ b/drivers/net/wireless/cisco/airo.c @@ -6067,8 +6067,7 @@ static int airo_get_nick(struct net_device *dev, struct airo_info *local = dev->ml_priv; readConfigRid(local, 1); - strncpy(extra, local->config.nodeName, 16); - extra[16] = '\0'; + strscpy_pad(extra, local->config.nodeName, IW_ESSID_MAX_SIZE); dwrq->length = strlen(extra); return 0;