linux-kernel - Re: [PATCH 08/18] iommu: Introduce cache

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20190411100235.GQ9224@smile.fi.intel.com>
Date:   Thu, 11 Apr 2019 13:02:35 +0300
From:   Andriy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Jacob Pan <jacob.jun.pan@...ux.intel.com>
Cc:     iommu@...ts.linux-foundation.org,
        LKML <linux-kernel@...r.kernel.org>,
        Joerg Roedel <joro@...tes.org>,
        David Woodhouse <dwmw2@...radead.org>,
        Alex Williamson <alex.williamson@...hat.com>,
        Jean-Philippe Brucker <jean-philippe.brucker@....com>,
        Yi Liu <yi.l.liu@...el.com>,
        "Tian, Kevin" <kevin.tian@...el.com>,
        Raj Ashok <ashok.raj@...el.com>,
        Christoph Hellwig <hch@...radead.org>,
        Lu Baolu <baolu.lu@...ux.intel.com>,
        "Liu, Yi L" <yi.l.liu@...ux.intel.com>, Liu@...le.fi.intel.com,
        Eric Auger <eric.auger@...hat.com>
Subject: Re: [PATCH 08/18] iommu: Introduce cache_invalidate API

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@...ux.intel.com> 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@...ux.intel.com> 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