Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 4 Aug 2021 17:23:54 -0500
From:   Bjorn Andersson <bjorn.andersson@linaro.org>
To:     Len Baker <len.baker@gmx.com>
Cc:     Kees Cook <keescook@chromium.org>, Andy Gross <agross@kernel.org>,
        Geert Uytterhoeven <geert+renesas@glider.be>,
        Magnus Damm <magnus.damm@gmail.com>,
        Santosh Shilimkar <ssantosh@kernel.org>,
        David Laight <David.Laight@aculab.com>,
        Robin Murphy <robin.murphy@arm.com>,
        linux-hardening@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v3] drivers/soc: Remove all strcpy() uses
Message-ID: <YQsTesvLfAwd8z5B@builder.lan>
References: <20210801131958.6144-1-len.baker@gmx.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210801131958.6144-1-len.baker@gmx.com>
Precedence: bulk

On Sun 01 Aug 08:19 CDT 2021, Len Baker wrote:

> strcpy() performs no bounds checking on the destination buffer. This
> could result in linear overflows beyond the end of the buffer, leading
> to all kinds of misbehaviors. The safe replacement is strscpy().
> 

While this is true, are any of these uses of strcpy affected by its
shortcomings?

> Moreover, when the size of the destination buffer cannot be obtained
> using "sizeof", use the memcpy function instead of strscpy.
> 

This is not why you're using memcpy, you're using it because you _know_
how many bytes should be copied - because you just did a strlen() and
allocated that amount of space.

> Signed-off-by: Len Baker <len.baker@gmx.com>
> ---
> This is a task of the KSPP [1]
> 
> [1] https://github.com/KSPP/linux/issues/88
> 
> Changelog v1 -> v2
> - Change the "area_name_size" variable for a shorter name (Geert
>   Uytterhoeven).
> - Add the "Reviewed-by: Geert Uytterhoeven" tag.
> - Use the memcpy function instead of strscpy function when the
>   size of the destination buffer cannot be obtained with "sizeof"
>   (David Laight, Robin Murphy).
> 
> Changelog v2 -> v3
> - Remove the "Reviewed-by: Geert Uytterhoeven" tag since the code
>   has changed after the v1 review (use of memcpy instead of
>   strscpy).
> 
>  drivers/soc/qcom/pdr_interface.c    | 13 +++++++------
>  drivers/soc/renesas/r8a779a0-sysc.c |  6 ++++--
>  drivers/soc/renesas/rcar-sysc.c     |  6 ++++--
>  drivers/soc/ti/knav_dma.c           |  2 +-
>  4 files changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/soc/qcom/pdr_interface.c b/drivers/soc/qcom/pdr_interface.c
> index 915d5bc3d46e..cf119fde749d 100644
> --- a/drivers/soc/qcom/pdr_interface.c
> +++ b/drivers/soc/qcom/pdr_interface.c
> @@ -131,7 +131,7 @@ static int pdr_register_listener(struct pdr_handle *pdr,
>  		return ret;
> 
>  	req.enable = enable;
> -	strcpy(req.service_path, pds->service_path);
> +	strscpy(req.service_path, pds->service_path, sizeof(req.service_path));
> 
>  	ret = qmi_send_request(&pdr->notifier_hdl, &pds->addr,
>  			       &txn, SERVREG_REGISTER_LISTENER_REQ,
> @@ -257,7 +257,7 @@ static int pdr_send_indack_msg(struct pdr_handle *pdr, struct pdr_service *pds,
>  		return ret;
> 
>  	req.transaction_id = tid;
> -	strcpy(req.service_path, pds->service_path);
> +	strscpy(req.service_path, pds->service_path, sizeof(req.service_path));
> 
>  	ret = qmi_send_request(&pdr->notifier_hdl, &pds->addr,
>  			       &txn, SERVREG_SET_ACK_REQ,
> @@ -406,7 +406,7 @@ static int pdr_locate_service(struct pdr_handle *pdr, struct pdr_service *pds)
>  		return -ENOMEM;
> 
>  	/* Prepare req message */
> -	strcpy(req.service_name, pds->service_name);
> +	strscpy(req.service_name, pds->service_name, sizeof(req.service_name));
>  	req.domain_offset_valid = true;
>  	req.domain_offset = 0;
> 
> @@ -531,8 +531,8 @@ struct pdr_service *pdr_add_lookup(struct pdr_handle *pdr,
>  		return ERR_PTR(-ENOMEM);
> 
>  	pds->service = SERVREG_NOTIFIER_SERVICE;
> -	strcpy(pds->service_name, service_name);
> -	strcpy(pds->service_path, service_path);
> +	strscpy(pds->service_name, service_name, sizeof(pds->service_name));
> +	strscpy(pds->service_path, service_path, sizeof(pds->service_path));
>  	pds->need_locator_lookup = true;
> 
>  	mutex_lock(&pdr->list_lock);
> @@ -587,7 +587,8 @@ int pdr_restart_pd(struct pdr_handle *pdr, struct pdr_service *pds)
>  			break;
> 
>  		/* Prepare req message */
> -		strcpy(req.service_path, pds->service_path);
> +		strscpy(req.service_path, pds->service_path,
> +			sizeof(req.service_path));

There's no need to break this line.

Thanks,
Bjorn

>  		addr = pds->addr;
>  		break;
>  	}
> diff --git a/drivers/soc/renesas/r8a779a0-sysc.c b/drivers/soc/renesas/r8a779a0-sysc.c
> index d464ffa1be33..7410b9fa9846 100644
> --- a/drivers/soc/renesas/r8a779a0-sysc.c
> +++ b/drivers/soc/renesas/r8a779a0-sysc.c
> @@ -404,19 +404,21 @@ static int __init r8a779a0_sysc_pd_init(void)
>  	for (i = 0; i < info->num_areas; i++) {
>  		const struct r8a779a0_sysc_area *area = &info->areas[i];
>  		struct r8a779a0_sysc_pd *pd;
> +		size_t n;
> 
>  		if (!area->name) {
>  			/* Skip NULLified area */
>  			continue;
>  		}
> 
> -		pd = kzalloc(sizeof(*pd) + strlen(area->name) + 1, GFP_KERNEL);
> +		n = strlen(area->name) + 1;
> +		pd = kzalloc(sizeof(*pd) + n, GFP_KERNEL);
>  		if (!pd) {
>  			error = -ENOMEM;
>  			goto out_put;
>  		}
> 
> -		strcpy(pd->name, area->name);
> +		memcpy(pd->name, area->name, n);
>  		pd->genpd.name = pd->name;
>  		pd->pdr = area->pdr;
>  		pd->flags = area->flags;
> diff --git a/drivers/soc/renesas/rcar-sysc.c b/drivers/soc/renesas/rcar-sysc.c
> index 53387a72ca00..b0a80de34c98 100644
> --- a/drivers/soc/renesas/rcar-sysc.c
> +++ b/drivers/soc/renesas/rcar-sysc.c
> @@ -396,19 +396,21 @@ static int __init rcar_sysc_pd_init(void)
>  	for (i = 0; i < info->num_areas; i++) {
>  		const struct rcar_sysc_area *area = &info->areas[i];
>  		struct rcar_sysc_pd *pd;
> +		size_t n;
> 
>  		if (!area->name) {
>  			/* Skip NULLified area */
>  			continue;
>  		}
> 
> -		pd = kzalloc(sizeof(*pd) + strlen(area->name) + 1, GFP_KERNEL);
> +		n = strlen(area->name) + 1;
> +		pd = kzalloc(sizeof(*pd) + n, GFP_KERNEL);
>  		if (!pd) {
>  			error = -ENOMEM;
>  			goto out_put;
>  		}
> 
> -		strcpy(pd->name, area->name);
> +		memcpy(pd->name, area->name, n);
>  		pd->genpd.name = pd->name;
>  		pd->ch.chan_offs = area->chan_offs;
>  		pd->ch.chan_bit = area->chan_bit;
> diff --git a/drivers/soc/ti/knav_dma.c b/drivers/soc/ti/knav_dma.c
> index 591d14ebcb11..5f9816d317a5 100644
> --- a/drivers/soc/ti/knav_dma.c
> +++ b/drivers/soc/ti/knav_dma.c
> @@ -691,7 +691,7 @@ static int dma_init(struct device_node *cloud, struct device_node *dma_node)
>  	dma->max_rx_flow = max_rx_flow;
>  	dma->max_tx_chan = min(max_tx_chan, max_tx_sched);
>  	atomic_set(&dma->ref_count, 0);
> -	strcpy(dma->name, node->name);
> +	strscpy(dma->name, node->name, sizeof(dma->name));
>  	spin_lock_init(&dma->lock);
> 
>  	for (i = 0; i < dma->max_tx_chan; i++) {
> --
> 2.25.1
>