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]

Message-ID: <5B8DA87D05A7694D9FA63FD143655C1B94224C36@hasmsx108.ger.corp.intel.com>
Date:   Tue, 20 Feb 2018 20:26:45 +0000
From:   "Winkler, Tomas" <tomas.winkler@...el.com>
To:     Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.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 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