linux-kernel - Re: [PATCH 02/13] staging: greybus: Switch from strlcpy to strscpy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 16 Feb 2021 08:54:59 -0600
From:   Alex Elder <elder@...e.org>
To:     Kumar Kartikeya Dwivedi <memxor@...il.com>,
        devel@...verdev.osuosl.org, gregkh@...uxfoundation.org,
        linux-kernel@...r.kernel.org
Cc:     Ian Abbott <abbotti@....co.uk>,
        H Hartley Sweeten <hsweeten@...ionengravers.com>,
        Ioana Radulescu <ruxandra.radulescu@....com>,
        Ioana Ciornei <ioana.ciornei@....com>,
        Johan Hovold <johan@...nel.org>, Alex Elder <elder@...nel.org>,
        Vaibhav Agarwal <vaibhav.sr@...il.com>,
        Mark Greer <mgreer@...malcreek.com>,
        Rui Miguel Silva <rmfrfs@...il.com>,
        Viresh Kumar <vireshk@...nel.org>,
        Marc Dietrich <marvin24@....de>,
        Jens Frederich <jfrederich@...il.com>,
        Daniel Drake <dsd@...top.org>,
        Jon Nettleton <jon.nettleton@...il.com>,
        Larry Finger <Larry.Finger@...inger.net>,
        Florian Schilhabel <florian.c.schilhabel@...glemail.com>,
        Sudip Mukherjee <sudipm.mukherjee@...il.com>,
        Teddy Wang <teddy.wang@...iconmotion.com>,
        Stephen Rothwell <sfr@...b.auug.org.au>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Florian Fainelli <f.fainelli@...il.com>,
        Al Viro <viro@...iv.linux.org.uk>,
        William Cohen <wcohen@...hat.com>,
        Mike Rapoport <rppt@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Robert Richter <rric@...nel.org>, greybus-dev@...ts.linaro.org,
        ac100@...ts.launchpad.net, linux-tegra@...r.kernel.org,
        linux-fbdev@...r.kernel.org
Subject: Re: [PATCH 02/13] staging: greybus: Switch from strlcpy to strscpy

On 1/31/21 11:28 AM, Kumar Kartikeya Dwivedi wrote:
> strlcpy is marked as deprecated in Documentation/process/deprecated.rst,
> and there is no functional difference when the caller expects truncation
> (when not checking the return value). strscpy is relatively better as it
> also avoids scanning the whole source string.
> 
> This silences the related checkpatch warnings from:
> 5dbdb2d87c29 ("checkpatch: prefer strscpy to strlcpy")
> 
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@...il.com>

This is a good change.  But while you're at it, I would
appreciate if you would convert a few spots to use
sizeof(dest) rather than a fixed constant.  I will
point them out below.

If this is the *only* request for a change on your
series, please tell me that and I can sign off on
this without you implementing my suggestion.  But
if you post a version 2, please do what I describe.

Thanks.

					-Alex

> ---
>   drivers/staging/greybus/audio_helper.c   | 2 +-
>   drivers/staging/greybus/audio_module.c   | 2 +-
>   drivers/staging/greybus/audio_topology.c | 6 +++---
>   drivers/staging/greybus/power_supply.c   | 2 +-
>   drivers/staging/greybus/spilib.c         | 4 ++--
>   5 files changed, 8 insertions(+), 8 deletions(-)

. . .


> diff --git a/drivers/staging/greybus/audio_module.c b/drivers/staging/greybus/audio_module.c
> index a243d60f0..0f9fdc077 100644
> --- a/drivers/staging/greybus/audio_module.c
> +++ b/drivers/staging/greybus/audio_module.c
> @@ -342,7 +342,7 @@ static int gb_audio_probe(struct gb_bundle *bundle,
>   	/* inform above layer for uevent */
>   	dev_dbg(dev, "Inform set_event:%d to above layer\n", 1);
>   	/* prepare for the audio manager */
> -	strlcpy(desc.name, gbmodule->name, GB_AUDIO_MANAGER_MODULE_NAME_LEN);
> +	strscpy(desc.name, gbmodule->name, GB_AUDIO_MANAGER_MODULE_NAME_LEN);

Please use this here instead:

	strscpy(desc.name, gbmodule->name, sizeof(desc.name));

>   	desc.vid = 2; /* todo */
>   	desc.pid = 3; /* todo */
>   	desc.intf_id = gbmodule->dev_id;
> diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c
> index 662e3e8b4..e816e4db5 100644
> --- a/drivers/staging/greybus/audio_topology.c
> +++ b/drivers/staging/greybus/audio_topology.c
> @@ -200,7 +200,7 @@ static int gbcodec_mixer_ctl_info(struct snd_kcontrol *kcontrol,
>   			return -EINVAL;
>   		name = gbaudio_map_controlid(module, data->ctl_id,
>   					     uinfo->value.enumerated.item);
> -		strlcpy(uinfo->value.enumerated.name, name, NAME_SIZE);
> +		strscpy(uinfo->value.enumerated.name, name, NAME_SIZE);

Please use this here instead:

		strscpy(uinfo->value.enumerated.name, name,
			sizeof(uinfo->valiue.enumerated.name));

(I know NAME_SIZE is used throughout this file, and
could also be converted in this way, but we can save
that for another patch.)

>   		break;
>   	default:
>   		dev_err(comp->dev, "Invalid type: %d for %s:kcontrol\n",
> @@ -1047,7 +1047,7 @@ static int gbaudio_tplg_create_widget(struct gbaudio_module_info *module,
>   	}
>   
>   	/* Prefix dev_id to widget control_name */
> -	strlcpy(temp_name, w->name, NAME_SIZE);
> +	strscpy(temp_name, w->name, NAME_SIZE);

Please use this here instead:

	strscpy(temp_name, w->name, sizeof(temp_name));

>   	snprintf(w->name, NAME_SIZE, "GB %d %s", module->dev_id, temp_name);
>   
>   	switch (w->type) {
> @@ -1169,7 +1169,7 @@ static int gbaudio_tplg_process_kcontrols(struct gbaudio_module_info *module,
>   		}
>   		control->id = curr->id;
>   		/* Prefix dev_id to widget_name */
> -		strlcpy(temp_name, curr->name, NAME_SIZE);
> +		strscpy(temp_name, curr->name, NAME_SIZE);


Please use this here instead:

		strscpy(temp_name, curr->name, sizeof(temp_name));

>   		snprintf(curr->name, NAME_SIZE, "GB %d %s", module->dev_id,
>   			 temp_name);
>   		control->name = curr->name;

. . .