linux-kernel - Re: [PATCH v3] tpm: add support for partial reads

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: <825f4c01-efb1-5034-ea90-9ea5ea8f5b45@intel.com>
Date:   Thu, 15 Nov 2018 16:26:33 -0800
From:   Tadeusz Struk <tadeusz.struk@...el.com>
To:     Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
Cc:     jgg@...pe.ca, linux-integrity@...r.kernel.org,
        linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] tpm: add support for partial reads

On 11/15/18 3:31 PM, Jarkko Sakkinen wrote:
> You could drop these memset() calls and also one from
> tpm_timeout_work(). The call could be done once in the beginning of
> tpm_common_write() instead of having three different call sites.
> 

Don't we want to clean the buffer as the response is read?
When we will only memset it in write and if one would send
just one command there will only be one response.
The data will sit in the buffer until the next command.
There will be a quite bit time window when the data can leak.

> 
> Naming becomes a mess and the comment for data_pending variable is
> incorrect as it is also used for synchronous operation.
> 
> Maybe add a prepending commit to rename it as
> 
> 	/* Holds the resul of the tpm_transmit() last call. */
> 	ssize_t transmit_result;

Agree, will do that.

> 
> That is at least clear and obvious on what it contains.
> 
> The comment for partial_data is incorrect as the variable does not
> contain any data.

Yes, I will change it.

> 
> We could use declare:
> 
> 	/* Holds the count how much of the response is still unread. */
> 	size_t response_pending;
> 
> Observe another remark from your commit: there is no reaso to ssize_t as
> the type as the value should never be a negative number.

Yes, in fact now the priv->data_pending can also be only positive.
I'll change it and send a new version soon.
Thanks,
-- 
Tadeusz