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@xxxxxxxxx>
> > ---
> >   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;