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@xxxxxxx> > --- > 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 >
