Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Message-ID: <d4157726d924a3ddad477923d6bcb4a8e6a55e60.camel@HansenPartnership.com>
Subject: Re: [PATCH v3 4/6] tpm: Support TPM2 sized buffers (TPM2B)
From:   James Bottomley <James.Bottomley@HansenPartnership.com>
To:     Jarkko Sakkinen <jarkko@kernel.org>,
        linux-integrity@vger.kernel.org
Cc:     keyrings@vger.kernel.org,
        William Roberts <bill.c.roberts@gmail.com>,
        Stefan Berger <stefanb@linux.ibm.com>,
        David Howells <dhowells@redhat.com>,
        Jason Gunthorpe <jgg@ziepe.ca>,
        Mimi Zohar <zohar@linux.ibm.com>,
        Peter Huewe <peterhuewe@gmx.de>,
        Paul Moore <paul@paul-moore.com>,
        James Morris <jmorris@namei.org>,
        "Serge E. Hallyn" <serge@hallyn.com>,
        Jerry Snitselaar <jsnitsel@redhat.com>,
        Mario Limonciello <mario.limonciello@amd.com>,
        Julien Gomes <julien@arista.com>,
        open list <linux-kernel@vger.kernel.org>,
        "open list:SECURITY SUBSYSTEM" 
        <linux-security-module@vger.kernel.org>
Date:   Fri, 27 Oct 2023 08:32:42 -0400
In-Reply-To: <20231024011531.442587-5-jarkko@kernel.org>
References: <20231024011531.442587-1-jarkko@kernel.org>
         <20231024011531.442587-5-jarkko@kernel.org>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.42.4 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Tue, 2023-10-24 at 04:15 +0300, Jarkko Sakkinen wrote:
> +++ b/drivers/char/tpm/tpm-buf.c
> @@ -7,22 +7,32 @@
>  #include <linux/tpm.h>
>  
>  /**
> - * tpm_buf_init() - Initialize from the heap
> + * tpm_buf_init() - Initialize a TPM buffer
>   * @buf:       A @tpm_buf
> + * @sized:     Represent a sized buffer (TPM2B)
> + * @alloc:     Allocate from the heap
>   *
>   * Initialize all structure fields to zero, allocate a page from the
> heap, and
>   * zero the bytes that the buffer headers will consume.
>   *
>   * Return: 0 or -ENOMEM
>   */
> -int tpm_buf_init(struct tpm_buf *buf)
> +int tpm_buf_init(struct tpm_buf *buf, bool alloc, bool sized)

I think it creates a phenomenally confusing interface to use multiple
booleans because, unlike flags, it's not self describing at point of
use.  The confusion is enormously heightened here by having the doc
book arguments be the reverse of the actual function prototype (I just
tripped over this).

The alloc flag is particularly counter intuitive: if you pass in an
allocated buffer, you expect to be responsible for freeing it again,
but that's not how you use it; you really use it like a reset not an
alloc, which looks odd because you already created a separate
tpm_buf_reset function which can't be used in this case.

Why not replace the alloc flags with two reset functions: one for TPM2B
buffers and one for command buffers?

James