Message ID | 20230726-asoc-intel-skylake-remove-deprecated-strncpy-v1-1-020e04184c7d@google.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:a985:0:b0:3e4:2afc:c1 with SMTP id t5csp680533vqo; Wed, 26 Jul 2023 14:43:38 -0700 (PDT) X-Google-Smtp-Source: APBJJlE5ggGt7p9AuMFZuIxKuWFU6gbuTrviFdeAcduLIPC7PET0mzQYCUl2d95m2rhu3gEsmulT X-Received: by 2002:a17:903:11c8:b0:1b8:b285:ec96 with SMTP id q8-20020a17090311c800b001b8b285ec96mr4207706plh.23.1690407818267; Wed, 26 Jul 2023 14:43:38 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1690407818; cv=none; d=google.com; s=arc-20160816; b=GtBTdrrNjFtg9SN4krLnKVp1dWBOR1q6ja30iJbpIN0wTwLlsLyjXDIP6ATqi4VE1G rKCabDhsIRt9xKDAPK1Epwv85xlTkzkH8XTkSoWkTIVQhr6NzAVfNxEXxA7XODBvYyIr 65kb/uocr33baz+VgUaEp/j4F1tXSMy636SZN9l7y8U1fNs3y2KO6t4niLqd9fYQm02W 3TTL30oqLLrGSRTVGOROci5dN4oDJ8z6KFsFKNo+39NcEi/c7kKB8tICxlYhgCI1Pkzc R58ubWAvzOq8sT23ldhLvM4KWIC474XMGHe/mYI3mlMp5kfyXYpoA/pLBf8JRdqEYHg3 /+PA== 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=rZM5DChdNI8S3fRaG82Uf+jm5x9WWsFq+xNzYHKGX4I=; fh=WUONaSrVESbtxTUxKz3PSmNG5QuGmvMLGfZSLF3EqqA=; b=PNo7XeovRHK8FitZxPfJabwQREUrZKW0U7Dh0u/XLXAO8djC7cVqB4d3r/A23+N01F nYyTDKu381SWuNxnLU4R0WS7iCLQ17TdVIvkAKJXtCTnyO/pltNxAHuS8Kbiro/BfYf4 Bk7G8+oeJFNd9/o5EC4fiBJV9PzTe4goUtk8gVmjBI2yYdTOvWQuzW8YvH+59HOO9bDT 9Oe4L43GCMpcMQbUbyegB32rqi9BWL+s9qwE54EvZJ4vLl/VQTKj1seQ4F7ayzBSd/n6 hwCMZPIRR5zoPUEzejvLu5gWFQa/+zmqimKQnyqUb2K3iWs1KQxWqyoQi8czWGYnSsX2 Lfng== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20221208 header.b=TFvfXD2j; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 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 (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id k5-20020a170902d58500b001bb95a5cb9fsi11776plh.522.2023.07.26.14.43.24; Wed, 26 Jul 2023 14:43:38 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@google.com header.s=20221208 header.b=TFvfXD2j; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229798AbjGZVMW (ORCPT <rfc822;kloczko.tomasz@gmail.com> + 99 others); Wed, 26 Jul 2023 17:12:22 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40744 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229487AbjGZVMU (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 26 Jul 2023 17:12:20 -0400 Received: from mail-yb1-xb4a.google.com (mail-yb1-xb4a.google.com [IPv6:2607:f8b0:4864:20::b4a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9BE691BE8 for <linux-kernel@vger.kernel.org>; Wed, 26 Jul 2023 14:12:19 -0700 (PDT) Received: by mail-yb1-xb4a.google.com with SMTP id 3f1490d57ef6-c6dd0e46a52so184301276.2 for <linux-kernel@vger.kernel.org>; Wed, 26 Jul 2023 14:12:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1690405939; x=1691010739; h=cc:to:from:subject:message-id:mime-version:date:from:to:cc:subject :date:message-id:reply-to; bh=rZM5DChdNI8S3fRaG82Uf+jm5x9WWsFq+xNzYHKGX4I=; b=TFvfXD2jpjpWq8sY4RDHzUNEf2ymN+fSetV8uTMDOQ+efdSDZm9AVu17Ww7vnobhgB ipCdmG9oLkCSmc/Ewxmh9qpvD8J5WxmQPeRTIimCFCbthp+v2w4LMNk9MqelA3mbqiTx b6yNHsrXMJYrky0IG18TgFNI2T0u0kk++fomF7W7zJ1tSMZaclcN9/xsXE1jSPnWxbIt SD8OnfWzUGmqz8Li/3PhJb77U/8kU8ZqMf4watsGPkU4z89D+rJiwf62psgJ7kJGdK8l Jy2wFbXod1DCUCD/DJQnR8q+eZ1PFAyayDYnC0l5FzvVGZBLA3+9PgSKC+VLpfHFrThr Smow== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1690405939; x=1691010739; h=cc:to:from:subject:message-id:mime-version:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=rZM5DChdNI8S3fRaG82Uf+jm5x9WWsFq+xNzYHKGX4I=; b=E99CINyJK3mRro6rzmDf29iavcCNZ15AJZ1xOvXwfxDberf5DpqZc2DwFjzpMPOhVV upja0/505usyH0dg+zIBYOVHbFy5L/yG7lbrkYVKmax0JbAADeEoftjEmLG5PVeC0Z0N BXp5y0QJ5eDQSivzvxbwQrzTfn3KmMIaRRWuA5mxSna/fqrZU+jtBTqVQcZp4F/sHv0C E/7neLMyPQPiO+hkebSaoC5beg09V3e6JdaiiFTMZ0jFSD92sv2DInG8fcCrq0+a3z6a 4zf5Q0gLCNyaXeBuwO91hKL5Q65lz4VwYw5WGv+UYNWkcokqsJDEKdeAUzBD0+YZJJKu jujg== X-Gm-Message-State: ABy/qLYYJUHLNCOAzG1PyI05EyL+B/BaS8tFN5nNAfuEeOxum8MVS0AH qdGpfEWFSoLD19HaMuu9gOwm8Mu7IKQNnBCHAA== X-Received: from jstitt-linux1.c.googlers.com ([fda3:e722:ac3:cc00:2b:ff92:c0a8:23b5]) (user=justinstitt job=sendgmr) by 2002:a25:fe05:0:b0:d10:c94a:be7e with SMTP id k5-20020a25fe05000000b00d10c94abe7emr19783ybe.8.1690405938894; Wed, 26 Jul 2023 14:12:18 -0700 (PDT) Date: Wed, 26 Jul 2023 21:12:18 +0000 Mime-Version: 1.0 X-B4-Tracking: v=1; b=H4sIADGMwWQC/52NQQ6DIBBFr2JYdxqgamNXvUfjgg6DEhUMEFJjv HupR+jy/fy8t7NIwVJkj2pngbKN1rsC4lIxHJUbCKwuzCSXN36XLajoEaxLNEOctllNBIEWnwk 0rYFQJdIQU3C4btDpNxqULa85sqIsB2M/Z+7VFx5tTD5sZz2L3/pnKAsQoNuaeI1dYxrzHLwfZ rqiX1h/HMcXwpnJEOkAAAA= X-Developer-Key: i=justinstitt@google.com; a=ed25519; pk=tC3hNkJQTpNX/gLKxTNQKDmiQl6QjBNCGKJINqAdJsE= X-Developer-Signature: v=1; a=ed25519-sha256; t=1690405938; l=2258; i=justinstitt@google.com; s=20230717; h=from:subject:message-id; bh=lQAXd/QKEocVTpWzsLq/yuRWIfGT/6JXnq+xx86jdyo=; b=qfHnDTc0z2fwrsubm0mfrEuvyffKkOeKhwokW9XrOjcVXFcWc7iFlQrcWEH+KOMyhxzifxMK8 aycPeRwhO0xD3vZ3vBW2pcHncCaCAkivn+JIrWDtsLx0CBm1ilapKZ+ X-Mailer: b4 0.12.3 Message-ID: <20230726-asoc-intel-skylake-remove-deprecated-strncpy-v1-1-020e04184c7d@google.com> Subject: [PATCH] ASoC: Intel: Skylake: replace deprecated strncpy with strscpy From: justinstitt@google.com To: Cezary Rojewski <cezary.rojewski@intel.com>, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>, Liam Girdwood <liam.r.girdwood@linux.intel.com>, Peter Ujfalusi <peter.ujfalusi@linux.intel.com>, Bard Liao <yung-chuan.liao@linux.intel.com>, Ranjani Sridharan <ranjani.sridharan@linux.intel.com>, Kai Vehmanen <kai.vehmanen@linux.intel.com>, Mark Brown <broonie@kernel.org>, Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com> Cc: Kees Cook <keescook@chromium.org>, Nathan Chancellor <nathan@kernel.org>, alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org, Justin Stitt <justinstitt@google.com> Content-Type: text/plain; charset="utf-8" X-Spam-Status: No, score=-9.6 required=5.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE,USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1772521068450985694 X-GMAIL-MSGID: 1772521068450985694 |
Series |
ASoC: Intel: Skylake: replace deprecated strncpy with strscpy
|
|
Commit Message
Justin Stitt
July 26, 2023, 9:12 p.m. UTC
`strncpy` is deprecated for use on NUL-terminated destination strings [1].
A suitable replacement is `strscpy` [2] due to the fact that it
guarantees NUL-termination on its destination buffer argument which is
_not_ the case for `strncpy`!
It was pretty difficult, in this case, to try and figure out whether or
not the destination buffer was zero-initialized. If it is and this
behavior is relied on then perhaps `strscpy_pad` is the preferred
option here.
Kees was able to help me out and identify the following code snippet
which seems to show that the destination buffer is zero-initialized.
| skl = devm_kzalloc(&pci->dev, sizeof(*skl), GFP_KERNEL);
With this information, I opted for `strscpy` since padding is seemingly
not required.
Also within this patch is a change to an instance of `x > y - 1` to `x >= y`
which tends to be more robust and readable. Consider, for instance, if `y` was
somehow `INT_MIN`.
[1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
[2]: manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html
Link: https://github.com/KSPP/linux/issues/90
Suggested-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
sound/soc/intel/skylake/skl-topology.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
---
base-commit: 0b4a9fdc9317440a71d4d4c264a5650bf4a90f3c
change-id: 20230726-asoc-intel-skylake-remove-deprecated-strncpy-9dbcfc26040c
Best regards,
Comments
On Wed, Jul 26, 2023 at 09:12:18PM +0000, justinstitt@google.com wrote: > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > A suitable replacement is `strscpy` [2] due to the fact that it > guarantees NUL-termination on its destination buffer argument which is > _not_ the case for `strncpy`! > > It was pretty difficult, in this case, to try and figure out whether or > not the destination buffer was zero-initialized. If it is and this > behavior is relied on then perhaps `strscpy_pad` is the preferred > option here. > > Kees was able to help me out and identify the following code snippet > which seems to show that the destination buffer is zero-initialized. > > | skl = devm_kzalloc(&pci->dev, sizeof(*skl), GFP_KERNEL); > > With this information, I opted for `strscpy` since padding is seemingly > not required. We did notice that str_elem->string is 44 bytes, but skl->lib_info[ref_count].name is 128 bytes. If str_elem->string isn't NUL-terminated, this can still hit an over-read condition (though CONFIG_FORTIFY_SOURCE would have caught it both before with strncpy() and now with strscpy()). So I assume it is expected to be NUL-terminated? > Also within this patch is a change to an instance of `x > y - 1` to `x >= y` > which tends to be more robust and readable. Consider, for instance, if `y` was > somehow `INT_MIN`. I'd split this change into a separate patch -- it's logically unrelated (but seems a reasonable cleanup). > > [1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > [2]: manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html > > Link: https://github.com/KSPP/linux/issues/90 > Suggested-by: Kees Cook <keescook@chromium.org> > Signed-off-by: Justin Stitt <justinstitt@google.com> > --- > sound/soc/intel/skylake/skl-topology.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c > index 96cfebded072..67f08ec3a2ea 100644 > --- a/sound/soc/intel/skylake/skl-topology.c > +++ b/sound/soc/intel/skylake/skl-topology.c > @@ -3154,12 +3154,12 @@ static int skl_tplg_fill_str_mfest_tkn(struct device *dev, > > switch (str_elem->token) { > case SKL_TKN_STR_LIB_NAME: > - if (ref_count > skl->lib_count - 1) { > + if (ref_count >= skl->lib_count) { > ref_count = 0; > return -EINVAL; > } > > - strncpy(skl->lib_info[ref_count].name, > + strscpy(skl->lib_info[ref_count].name, > str_elem->string, > ARRAY_SIZE(skl->lib_info[ref_count].name)); > ref_count++; > > --- > base-commit: 0b4a9fdc9317440a71d4d4c264a5650bf4a90f3c > change-id: 20230726-asoc-intel-skylake-remove-deprecated-strncpy-9dbcfc26040c > > Best regards, > -- > Justin Stitt <justinstitt@google.com> >
On 7/27/2023 12:34 AM, Kees Cook wrote: > On Wed, Jul 26, 2023 at 09:12:18PM +0000, justinstitt@google.com wrote: >> `strncpy` is deprecated for use on NUL-terminated destination strings [1]. >> >> A suitable replacement is `strscpy` [2] due to the fact that it >> guarantees NUL-termination on its destination buffer argument which is >> _not_ the case for `strncpy`! >> >> It was pretty difficult, in this case, to try and figure out whether or >> not the destination buffer was zero-initialized. If it is and this >> behavior is relied on then perhaps `strscpy_pad` is the preferred >> option here. >> >> Kees was able to help me out and identify the following code snippet >> which seems to show that the destination buffer is zero-initialized. >> >> | skl = devm_kzalloc(&pci->dev, sizeof(*skl), GFP_KERNEL); >> >> With this information, I opted for `strscpy` since padding is seemingly >> not required. > > We did notice that str_elem->string is 44 bytes, but > skl->lib_info[ref_count].name is 128 bytes. If str_elem->string isn't > NUL-terminated, this can still hit an over-read condition (though > CONFIG_FORTIFY_SOURCE would have caught it both before with strncpy() > and now with strscpy()). So I assume it is expected to be > NUL-terminated? > Yes it is a filename of additional library which can be loaded, topology UAPI only allows for passing 44 bytes long strings per string token (see snd_soc_tplg_vendor_array -> union -> string flex array -> snd_soc_tplg_vendor_string_elem -> SNDRV_CTL_ELEM_ID_NAME_MAXLEN), so we could also change length of skl->lib_info[ref_count].name and potentially save few bytes. And looking at it again I also think that we should not copy destination size number of bytes, by which I mean ARRAY_SIZE(skl->lib_info[ref_count].name), which is 128 in this case... so either need to change destination buffer size to be same as topology field or calculate it differently.
On Fri, Jul 28, 2023 at 09:25:24AM +0200, Amadeusz Sławiński wrote: > On 7/27/2023 12:34 AM, Kees Cook wrote: > > On Wed, Jul 26, 2023 at 09:12:18PM +0000, justinstitt@google.com wrote: > > > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > > > > > A suitable replacement is `strscpy` [2] due to the fact that it > > > guarantees NUL-termination on its destination buffer argument which is > > > _not_ the case for `strncpy`! > > > > > > It was pretty difficult, in this case, to try and figure out whether or > > > not the destination buffer was zero-initialized. If it is and this > > > behavior is relied on then perhaps `strscpy_pad` is the preferred > > > option here. > > > > > > Kees was able to help me out and identify the following code snippet > > > which seems to show that the destination buffer is zero-initialized. > > > > > > | skl = devm_kzalloc(&pci->dev, sizeof(*skl), GFP_KERNEL); > > > > > > With this information, I opted for `strscpy` since padding is seemingly > > > not required. > > > > We did notice that str_elem->string is 44 bytes, but > > skl->lib_info[ref_count].name is 128 bytes. If str_elem->string isn't > > NUL-terminated, this can still hit an over-read condition (though > > CONFIG_FORTIFY_SOURCE would have caught it both before with strncpy() > > and now with strscpy()). So I assume it is expected to be > > NUL-terminated? > > > > Yes it is a filename of additional library which can be loaded, topology > UAPI only allows for passing 44 bytes long strings per string token (see > snd_soc_tplg_vendor_array -> union -> string flex array -> > snd_soc_tplg_vendor_string_elem -> SNDRV_CTL_ELEM_ID_NAME_MAXLEN), so we Thanks for the details! And just to confirm, these are (expected to be) NUL-terminated? > could also change length of > skl->lib_info[ref_count].name and potentially save few bytes. And looking at > it again I also think that we should not copy destination size number of > bytes, by which I mean ARRAY_SIZE(skl->lib_info[ref_count].name), which is > 128 in this case... so either need to change destination buffer size to be > same as topology field or calculate it differently. If the source is NUL-terminated, it's fine as-is. (And CONFIG_FORTIFY_SOURCE will catch problems if not.)
diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c index 96cfebded072..67f08ec3a2ea 100644 --- a/sound/soc/intel/skylake/skl-topology.c +++ b/sound/soc/intel/skylake/skl-topology.c @@ -3154,12 +3154,12 @@ static int skl_tplg_fill_str_mfest_tkn(struct device *dev, switch (str_elem->token) { case SKL_TKN_STR_LIB_NAME: - if (ref_count > skl->lib_count - 1) { + if (ref_count >= skl->lib_count) { ref_count = 0; return -EINVAL; } - strncpy(skl->lib_info[ref_count].name, + strscpy(skl->lib_info[ref_count].name, str_elem->string, ARRAY_SIZE(skl->lib_info[ref_count].name)); ref_count++;