| Message ID | 1539885344-18974-4-git-send-email-talho@nvidia.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | BPMP-FW version query ("tag") update | expand |
On Thu, Oct 18, 2018 at 08:55:42PM +0300, Timo Alho wrote: > Last two characters of the version tag that is 32 bytes long were > stripped out. > > Signed-off-by: Timo Alho <talho@nvidia.com> > --- > drivers/firmware/tegra/bpmp.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) Acked-by: Sivaram Nair <sivaramn@nvidia.com>
On 18/10/2018 18:55, Timo Alho wrote: > Last two characters of the version tag that is 32 bytes long were > stripped out. > > Signed-off-by: Timo Alho <talho@nvidia.com> > --- > drivers/firmware/tegra/bpmp.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/firmware/tegra/bpmp.c b/drivers/firmware/tegra/bpmp.c > index 79859ab..a7d8954 100644 > --- a/drivers/firmware/tegra/bpmp.c > +++ b/drivers/firmware/tegra/bpmp.c > @@ -558,8 +558,9 @@ static int tegra_bpmp_get_firmware_tag(struct tegra_bpmp *bpmp, char *tag, > dma_addr_t phys; > void *virt; > int err; > + const size_t tag_sz = 32; Why not a #define for this? > > - virt = dma_alloc_coherent(bpmp->dev, MSG_DATA_MIN_SZ, &phys, > + virt = dma_alloc_coherent(bpmp->dev, tag_sz, &phys, > GFP_KERNEL | GFP_DMA32); > if (!virt) > return -ENOMEM; > @@ -577,9 +578,9 @@ static int tegra_bpmp_get_firmware_tag(struct tegra_bpmp *bpmp, char *tag, > local_irq_restore(flags); > > if (err == 0) > - strlcpy(tag, virt, size); > + memcpy(tag, virt, min(size, tag_sz)); Should it be an error if size is less than 32? Otherwise characters could be silently stripped. Cheers Jon
On Fri, Oct 19, 2018 at 01:07:19PM +0100, Jon Hunter wrote: > > > On 18/10/2018 18:55, Timo Alho wrote: > > Last two characters of the version tag that is 32 bytes long were > > stripped out. > > > > Signed-off-by: Timo Alho <talho@nvidia.com> > > --- > > drivers/firmware/tegra/bpmp.c | 11 ++++++----- > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/firmware/tegra/bpmp.c b/drivers/firmware/tegra/bpmp.c > > index 79859ab..a7d8954 100644 > > --- a/drivers/firmware/tegra/bpmp.c > > +++ b/drivers/firmware/tegra/bpmp.c > > @@ -558,8 +558,9 @@ static int tegra_bpmp_get_firmware_tag(struct tegra_bpmp *bpmp, char *tag, > > dma_addr_t phys; > > void *virt; > > int err; > > + const size_t tag_sz = 32; > > Why not a #define for this? > > > > > - virt = dma_alloc_coherent(bpmp->dev, MSG_DATA_MIN_SZ, &phys, > > + virt = dma_alloc_coherent(bpmp->dev, tag_sz, &phys, > > GFP_KERNEL | GFP_DMA32); > > if (!virt) > > return -ENOMEM; > > @@ -577,9 +578,9 @@ static int tegra_bpmp_get_firmware_tag(struct tegra_bpmp *bpmp, char *tag, > > local_irq_restore(flags); > > > > if (err == 0) > > - strlcpy(tag, virt, size); > > + memcpy(tag, virt, min(size, tag_sz)); > > Should it be an error if size is less than 32? Otherwise characters > could be silently stripped. Is the tag guaranteed to always be 32 characters? If it was possible for it to be less, we'd now be copying out uninitialized characters, right? I think dma_alloc_coherent() zeroes memory, so this should be safe, but we should double-check to make sure we're not potentially leaking data here. Thierry
On 22.10.2018 12.37, Thierry Reding wrote: > On Fri, Oct 19, 2018 at 01:07:19PM +0100, Jon Hunter wrote: >> >> >> On 18/10/2018 18:55, Timo Alho wrote: >>> Last two characters of the version tag that is 32 bytes long were >>> stripped out. >>> >>> Signed-off-by: Timo Alho <talho@nvidia.com> >>> --- >>> drivers/firmware/tegra/bpmp.c | 11 ++++++----- >>> 1 file changed, 6 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/firmware/tegra/bpmp.c b/drivers/firmware/tegra/bpmp.c >>> index 79859ab..a7d8954 100644 >>> --- a/drivers/firmware/tegra/bpmp.c >>> +++ b/drivers/firmware/tegra/bpmp.c >>> @@ -558,8 +558,9 @@ static int tegra_bpmp_get_firmware_tag(struct tegra_bpmp *bpmp, char *tag, >>> dma_addr_t phys; >>> void *virt; >>> int err; >>> + const size_t tag_sz = 32; >> >> Why not a #define for this? >> >>> >>> - virt = dma_alloc_coherent(bpmp->dev, MSG_DATA_MIN_SZ, &phys, >>> + virt = dma_alloc_coherent(bpmp->dev, tag_sz, &phys, >>> GFP_KERNEL | GFP_DMA32); >>> if (!virt) >>> return -ENOMEM; >>> @@ -577,9 +578,9 @@ static int tegra_bpmp_get_firmware_tag(struct tegra_bpmp *bpmp, char *tag, >>> local_irq_restore(flags); >>> >>> if (err == 0) >>> - strlcpy(tag, virt, size); >>> + memcpy(tag, virt, min(size, tag_sz)); >> >> Should it be an error if size is less than 32? Otherwise characters >> could be silently stripped. > > Is the tag guaranteed to always be 32 characters? If it was possible for > it to be less, we'd now be copying out uninitialized characters, right? > I think dma_alloc_coherent() zeroes memory, so this should be safe, but > we should double-check to make sure we're not potentially leaking data > here. tag is always 32 characters. So I'll just update the code to check that 'size' parameter always equals to 32 to avoid any further complexity here. -Timo
diff --git a/drivers/firmware/tegra/bpmp.c b/drivers/firmware/tegra/bpmp.c index 79859ab..a7d8954 100644 --- a/drivers/firmware/tegra/bpmp.c +++ b/drivers/firmware/tegra/bpmp.c @@ -558,8 +558,9 @@ static int tegra_bpmp_get_firmware_tag(struct tegra_bpmp *bpmp, char *tag, dma_addr_t phys; void *virt; int err; + const size_t tag_sz = 32; - virt = dma_alloc_coherent(bpmp->dev, MSG_DATA_MIN_SZ, &phys, + virt = dma_alloc_coherent(bpmp->dev, tag_sz, &phys, GFP_KERNEL | GFP_DMA32); if (!virt) return -ENOMEM; @@ -577,9 +578,9 @@ static int tegra_bpmp_get_firmware_tag(struct tegra_bpmp *bpmp, char *tag, local_irq_restore(flags); if (err == 0) - strlcpy(tag, virt, size); + memcpy(tag, virt, min(size, tag_sz)); - dma_free_coherent(bpmp->dev, MSG_DATA_MIN_SZ, virt, phys); + dma_free_coherent(bpmp->dev, tag_sz, virt, phys); return err; } @@ -820,13 +821,13 @@ static int tegra_bpmp_probe(struct platform_device *pdev) goto free_mrq; } - err = tegra_bpmp_get_firmware_tag(bpmp, tag, sizeof(tag) - 1); + err = tegra_bpmp_get_firmware_tag(bpmp, tag, sizeof(tag)); if (err < 0) { dev_err(&pdev->dev, "failed to get firmware tag: %d\n", err); goto free_mrq; } - dev_info(&pdev->dev, "firmware: %s\n", tag); + dev_info(&pdev->dev, "firmware: %.*s\n", sizeof(tag), tag); platform_set_drvdata(pdev, bpmp);
Last two characters of the version tag that is 32 bytes long were stripped out. Signed-off-by: Timo Alho <talho@nvidia.com> --- drivers/firmware/tegra/bpmp.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)