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
Date: Wed, 4 Aug 2021 17:23:54 -0500 [thread overview]
Message-ID: <YQsTesvLfAwd8z5B@builder.lan> (raw)
In-Reply-To: <20210801131958.6144-1-len.baker@gmx.com>
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
>
next prev parent reply other threads:[~2021-08-04 22:24 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-01 13:19 [PATCH v3] drivers/soc: Remove all strcpy() uses Len Baker
2021-08-04 22:23 ` Bjorn Andersson [this message]
2021-08-07 17:48 ` Len Baker
2021-08-04 22:34 ` Bjorn Andersson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YQsTesvLfAwd8z5B@builder.lan \
--to=bjorn.andersson@linaro.org \
--cc=David.Laight@aculab.com \
--cc=agross@kernel.org \
--cc=geert+renesas@glider.be \
--cc=keescook@chromium.org \
--cc=len.baker@gmx.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-hardening@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=magnus.damm@gmail.com \
--cc=robin.murphy@arm.com \
--cc=ssantosh@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).