Re: [PATCH v2] tpm: Restore functionality to xen vtpm driver.

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 9/17/18 5:19 PM, Jarkko Sakkinen wrote:
> On Mon, Sep 17, 2018 at 09:54:37AM -0400, Boris Ostrovsky wrote:
>> On 9/16/18 3:25 PM, Jarkko Sakkinen wrote:
>>> On Thu, Sep 13, 2018 at 05:25:51PM -0400, Boris Ostrovsky wrote:
>>>> From: "Dr. Greg Wettstein" <greg@wind.enjellic.com>
>>>>
>>>> Functionality of the xen-tpmfront driver was lost secondary to
>>>> the introduction of xenbus multi-page support in commit ccc9d90a9a8b
>>>> ("xenbus_client: Extend interface to support multi-page ring").
>>>>
>>>> In this commit a pointer to the shared page address was being
>>>> passed to the xenbus_grant_ring() function rather then the
>>>> address of the shared page itself.  This resulted in a situation
>>> I'm sorry but I'm far from being expert with Xen and this sentence
>>> confuses me so maybe could open it up a bit.
>>>
>>> For me "shared page address" and "address of the shared page" are
>>> the same thing. What am I missing? I mean just different forms in
>>> english to describe the exact same thing...
>> xenbus_grant_ring() takes as an argument address of the ring shared
>> between two guests. What Greg was trying to describe was the fact that
>> existing code instead passes address of location where this address is
>> stored (i.e. somewhat similar to difference between pointer and pointer
>> to a pointer).
> Just to understand this bug better why did not the wrong version
> cause any undefined behavior? Sounds like a fatal bug. Does this
> cause crashes?

AFAIK, no, no crashes. I haven't tested this myself (and I believe
relatively few people use this functionality, which explains why this
has not been fixed for so long) but I don't think it will necessarily
crash. It's just that the frontend driver will be reading from wrong
location, causing TPM not to function properly.

Or maybe the frontend is writing but then I believe the write would
occur into tpm_private, and so will corrupt it. But the protocol will
fail right after this anyway.



>
>> Would this be better:
>>
>> "In this commit pointer to location of the where the shared page address
>> is stored was being passed to the xenbus_grant_ring() function rather
>> then the
>> address of the shared page itself."
> Yes, definitely!

OK, I'll send it shortly.


Thanks.
-boris


>
>> Or please suggest a better alternative, I'll be happy to amend the
>> commit message.
> Thank you.
>
>> Thanks.
>> -boris
> /Jarkko