[PATCH] iommu/dma: Potential uninitialized variable in iommu_dma_unmap_sg

[PATCH] iommu/dma: Potential uninitialized variable in iommu_dma_unmap_sg

[Alessandro Zanni]

This patch fix the possibility to have the variable 'start'
not initialized.

Smatch tool raises the error:
drivers/iommu/dma-iommu.c:1510
iommu_dma_unmap_sg() error: uninitialized symbol 'start'.

Signed-off-by: Alessandro Zanni <alessandro.zanni87@gmail.com>
---
 drivers/iommu/dma-iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 2a9fa0c8cc00..5b2596f4b24f 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -1496,7 +1496,7 @@ int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
 void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nents,
 		enum dma_data_direction dir, unsigned long attrs)
 {
-	dma_addr_t end = 0, start;
+	dma_addr_t end = 0, start = 0;
 	struct scatterlist *tmp;
 	int i;
 
-- 
2.43.0

Re: [PATCH] iommu/dma: Potential uninitialized variable in iommu_dma_unmap_sg

[Robin Murphy]

On 2024-10-02 9:31 am, Alessandro Zanni wrote:
> This patch fix the possibility to have the variable 'start'
> not initialized.

Why should it need initialising though? For "start" to never be set, 
then either sg_dma_is_bus_address() is true for the whole list, or the 
list is bogus and has sg_dma_len()==0 on the very first segment. Either 
way, the second loop will then do nothing, "if (end)" will remain false, 
and thus "start" will not be used. Where's the bug?

Thanks,
Robin.

> Smatch tool raises the error:
> drivers/iommu/dma-iommu.c:1510
> iommu_dma_unmap_sg() error: uninitialized symbol 'start'.
> 
> Signed-off-by: Alessandro Zanni <alessandro.zanni87@gmail.com>
> ---
>   drivers/iommu/dma-iommu.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 2a9fa0c8cc00..5b2596f4b24f 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -1496,7 +1496,7 @@ int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
>   void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nents,
>   		enum dma_data_direction dir, unsigned long attrs)
>   {
> -	dma_addr_t end = 0, start;
> +	dma_addr_t end = 0, start = 0;
>   	struct scatterlist *tmp;
>   	int i;
>   

Re: [PATCH] iommu/dma: Potential uninitialized variable in iommu_dma_unmap_sg

[Alessandro Zanni]

Hi,

this fix has been raised by a static analysis tool and it's more a similar to
a warning than a error/bug, even if the tool labels it as error.

I checked the code but, honestly, is quite hard to me to find a combination 
that might lead to an issue because it's strictly dependent on how the driver
works and the causes may be multiple, as you said: sg_dma_is_bus_address() 
true for all; sg_dma_len() false on the first; zero segments for the loop.

I sent this patch because maybe can be useful to avoid a possibile, unlikely,
combination that may lead to an error.

Up to you to decide either it's useful or not.

Thanks,
Alessandro

On 24/10/02 10:45, Robin Murphy wrote:
> On 2024-10-02 9:31 am, Alessandro Zanni wrote:
> > This patch fix the possibility to have the variable 'start'
> > not initialized.
> 
> Why should it need initialising though? For "start" to never be set, then
> either sg_dma_is_bus_address() is true for the whole list, or the list is
> bogus and has sg_dma_len()==0 on the very first segment. Either way, the
> second loop will then do nothing, "if (end)" will remain false, and thus
> "start" will not be used. Where's the bug?
> 
> Thanks,
> Robin.
> 
> > Smatch tool raises the error:
> > drivers/iommu/dma-iommu.c:1510
> > iommu_dma_unmap_sg() error: uninitialized symbol 'start'.
> > 
> > Signed-off-by: Alessandro Zanni <alessandro.zanni87@gmail.com>
> > ---
> >   drivers/iommu/dma-iommu.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> > index 2a9fa0c8cc00..5b2596f4b24f 100644
> > --- a/drivers/iommu/dma-iommu.c
> > +++ b/drivers/iommu/dma-iommu.c
> > @@ -1496,7 +1496,7 @@ int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
> >   void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nents,
> >   		enum dma_data_direction dir, unsigned long attrs)
> >   {
> > -	dma_addr_t end = 0, start;
> > +	dma_addr_t end = 0, start = 0;
> >   	struct scatterlist *tmp;
> >   	int i;