linux-kernel - Re: [PATCH 1/2 v3] tpm: cmd_ready command can be issued only after granting locality

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]

Date:   Wed, 21 Feb 2018 01:03:01 +0200
From:   Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
To:     "Winkler, Tomas" <tomas.winkler@...el.com>
Cc:     Jason Gunthorpe <jgg@...pe.ca>,
        "Usyskin, Alexander" <alexander.usyskin@...el.com>,
        "linux-integrity@...r.kernel.org" <linux-integrity@...r.kernel.org>,
        "linux-security-module@...r.kernel.org" 
        <linux-security-module@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2 v3] tpm: cmd_ready command can be issued only after
 granting locality

On Tue, Feb 20, 2018 at 08:26:45PM +0000, Winkler, Tomas wrote:
> > 
> > On Mon, 2018-02-19 at 11:43 +0000, Winkler, Tomas wrote:
> > > > All local variable declarations must be in the beginning of the
> > > > function.
> > >
> > > Who says?
> > 
> > It is coherent how we have everything else.
>  I will have to care about its value out of the scope where the variable existence is not relevant.
> 
> > It is much easier to see the stack allocation this way when the allocation is
> > only done in the beginning of each function. If you really need to do such
> > pattern, then it would be a better idea to consider an additional helper
> > function.
> The code block decides whether to modify 'rc'. I'm not sure if additional function will make
>  the code cleaner, on the opposite.
> > 
> > > > Your comment about not overriding error code is incorrect.
> > >
> > > Please explain?
> > 
> > 'l_rc' overrides 'rc' in the case when both are non-zero.
> 
> Yes, that's been the intention, we cannot return more than one value. 
> l_rc if set it has hire priority. 
> 
> > 
> > > > The value of 'rc' should be never overridden, which kind of supports
> > > > to "just print" behavior that we had for a locality error.
> > >
> > > You are not consistent, you've agreed with propagating it to user
> > > space.  The error will  be propagated in case of an error in locality
> > > relinquish the device is pretty much in non functional state and
> > > provious errors do not matter much, but rc value won't be modified if
> > > locality_reliquish succeeds.
> > 
> > Well, sometimes you fail to notice things and I failed to notice the collision
> > above. The commit message does not describe why 'l_rc'
> > overrides 'rc' in the case when both are non-zero. What was the reasoning,
> > which made you end up with this priority order?  Why is 'l_rc' more
> > important than 'rc'?
> 
> Because, it's fatal. I'm not sure it's matter much what the previous error was, it cannot be recovered
> That's my understanding of this flow.
> 
>  
> > My take is that does it really make sense have this change as part of a high
> > priority bug fix that should be as localized as possible?
> > Seems like a non-trivial problem by itself.
> 
> Yes, the issue here is that also an error path can fail. Now what is the correct return value.. 
> 
> In any case, in order to resolve this dispute, I will post a version when the error is just prints out,
> Once, however fatal the error is, it's very unlikely that it will happen.
> Second the driver will find the device not responding in a subsequent command.
> 
> Not perfect, but at least we will have functional driver.
> 
> Thanks
> Tomas
> 

Please add my tested by to next version. Thanks.

/Jarkko