linux-kernel - Re: [PATCH] iommu/dma: Potential uninitialized variable in iommu_dma_unmap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <5qpb5dotpef5livw36yok7vnbe7ylj5hg34azlpow6t5ca2xoh@sppdu63wbx3o>
Date: Wed, 2 Oct 2024 15:08:54 +0200
From: Alessandro Zanni <alessandro.zanni87@...il.com>
To: Robin Murphy <robin.murphy@....com>
Cc: joro@...tes.org, will@...nel.org, iommu@...ts.linux.dev, 
	linux-kernel@...r.kernel.org, skhan@...uxfoundation.org, anupnewsmail@...il.com
Subject: Re: [PATCH] iommu/dma: Potential uninitialized variable in
 iommu_dma_unmap_sg

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@...il.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;