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] [day] [month] [year] [list]

Message-ID: <20181116013341.GA26336@linux.intel.com>
Date:   Fri, 16 Nov 2018 03:33:41 +0200
From:   Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
To:     Tadeusz Struk <tadeusz.struk@...el.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 Thu, Nov 15, 2018 at 04:26:33PM -0800, Tadeusz Struk wrote:
> 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.

Point taken.

> > 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.

Right you're correct. Then it should be simply called as response_size
(and be size_t) as it is then the most precise name for what it
contains.

> Thanks,
> -- 
> Tadeusz

/Jarkko