Re: [PATCH 08/18] iommu: Introduce cache_invalidate API

[Andriy Shevchenko]

On Wed, Apr 10, 2019 at 02:21:31PM -0700, Jacob Pan wrote:
> On Tue, 9 Apr 2019 20:37:55 +0300
> Andriy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> > On Tue, Apr 09, 2019 at 09:43:28AM -0700, Jacob Pan wrote:
> > > On Tue, 9 Apr 2019 13:07:18 +0300
> > > Andriy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:  
> > > > On Mon, Apr 08, 2019 at 04:59:23PM -0700, Jacob Pan wrote:  
> > 
> > > > > +int iommu_cache_invalidate(struct iommu_domain *domain, struct
> > > > > device *dev,
> > > > > +			   struct iommu_cache_invalidate_info
> > > > > *inv_info) +{
> > > > > +	int ret = 0;    
> > > > 
> > > > Redundant assignment.
> > > >   
> > > I am not a security expert but initialization of local variable can
> > > be more secure.
> > > I was looking at this talk.
> > > https://outflux.net/slides/2018/lss/danger.pdf
> > > https://cwe.mitre.org/data/definitions/457.html  
> > 
> > I hardly see any of these applied to your case here.
> > Care to show what I'm missing?
> > 
> I thought your comments was that I should not need to initialize local
> variable ret = 0. 

Correct.

> Always initialize local variable can be a good
> security practice as suggested in the paper. Perhaps I missed
> something :)

Paper suggested to do that in a sense to avoid use of uninitialized variable.
This is not your case (usually it's not the case for variable which contains
return code), so, assignment is redundant. Moreover, default assignment can
hide an actual warning and an issue. Security people are not always correct.

> > > > > +	if (unlikely(!domain->ops->cache_invalidate))
> > > > > +		return -ENODEV;
> > > > > +
> > > > > +	ret = domain->ops->cache_invalidate(domain, dev,
> > > > > inv_info); +
> > > > > +	return ret;
> > > > > +}    

-- 
With Best Regards,
Andy Shevchenko