| 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
| ||
|
Date: Wed, 4 Aug 2021 17:23:54 -0500
From: Bjorn Andersson <bjorn.andersson@...aro.org>
To: Len Baker <len.baker@....com>
Cc: Kees Cook <keescook@...omium.org>, Andy Gross <agross@...nel.org>,
Geert Uytterhoeven <geert+renesas@...der.be>,
Magnus Damm <magnus.damm@...il.com>,
Santosh Shilimkar <ssantosh@...nel.org>,
David Laight <David.Laight@...lab.com>,
Robin Murphy <robin.murphy@....com>,
linux-hardening@...r.kernel.org, linux-arm-msm@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v3] drivers/soc: Remove all strcpy() uses
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@....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
>
Powered by blists - more mailing lists