`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]: http://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 <[email protected]>
Signed-off-by: Justin Stitt <[email protected]>
---
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 <[email protected]>
On Wed, Jul 26, 2023 at 09:12:18PM +0000, [email protected] 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]: http://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 <[email protected]>
> Signed-off-by: Justin Stitt <[email protected]>
> ---
> 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 <[email protected]>
>
--
Kees Cook
`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.
[1]: http://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 <[email protected]>
Signed-off-by: Justin Stitt <[email protected]>
---
Changes in v2:
- Remove extraneous logic change (thanks Kees)
- Link to v1: https://lore.kernel.org/r/20230726-asoc-intel-skylake-remove-deprecated-strncpy-v1-1-020e04184c7d@google.com
---
sound/soc/intel/skylake/skl-topology.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c
index 96cfebded072..0ead3ea605cd 100644
--- a/sound/soc/intel/skylake/skl-topology.c
+++ b/sound/soc/intel/skylake/skl-topology.c
@@ -3159,7 +3159,7 @@ static int skl_tplg_fill_str_mfest_tkn(struct device *dev,
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 <[email protected]>
On Thu, Jul 27, 2023 at 08:30:18PM +0000, Justin Stitt 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`!
Please don't send new patches in reply to old patches or serieses, this
makes it harder for both people and tools to understand what is going
on - it can bury things in mailboxes and make it difficult to keep track
of what current patches are, both for the new patches and the old ones.
On 7/27/2023 12:34 AM, Kees Cook wrote:
> On Wed, Jul 26, 2023 at 09:12:18PM +0000, [email protected] 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, [email protected] 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.)
--
Kees Cook
On Fri, Jul 28, 2023 at 11:56:08AM -0700, Kees Cook wrote:
> On Fri, Jul 28, 2023 at 12:27:24AM +0100, Mark Brown wrote:
> > Please don't send new patches in reply to old patches or serieses, this
> > makes it harder for both people and tools to understand what is going
> > on - it can bury things in mailboxes and make it difficult to keep track
> > of what current patches are, both for the new patches and the old ones.
> Hm, I see "X-Mailer: b4 0.12.3". Is this a default behavior of b4? (If
> so, that needs fixing.)
I've not noticed it doing that for my outbound patches and can't find
any option I tweaked to make it send as new threads, nor can I remember
configuring anything. There is a b4.send-same-thread option since v0.13
but it's default no according to the documentation:
https://b4.docs.kernel.org/en/latest/config.html#contributor-oriented-settings
On Fri, Jul 28, 2023 at 12:27:24AM +0100, Mark Brown wrote:
> On Thu, Jul 27, 2023 at 08:30:18PM +0000, Justin Stitt 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`!
>
> Please don't send new patches in reply to old patches or serieses, this
> makes it harder for both people and tools to understand what is going
> on - it can bury things in mailboxes and make it difficult to keep track
> of what current patches are, both for the new patches and the old ones.
Hm, I see "X-Mailer: b4 0.12.3". Is this a default behavior of b4? (If
so, that needs fixing.)
--
Kees Cook
On Thu, Jul 27, 2023 at 08:30:18PM +0000, Justin Stitt 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.
>
> [1]: http://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 <[email protected]>
> Signed-off-by: Justin Stitt <[email protected]>
Thanks for the updates! And based on the details from Amadeusz, it
looks safe.
Reviewed-by: Kees Cook <[email protected]>
--
Kees Cook