linux-kernel - Re: [PATCH 4/4] char/tpm: Less checks in tpm_ibmvtpm

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171019135632.4af42743@kitsune.suse.cz>
Date:   Thu, 19 Oct 2017 13:56:32 +0200
From:   Michal Suchánek <msuchanek@...e.de>
To:     SF Markus Elfring <elfring@...rs.sourceforge.net>
Cc:     linux-integrity@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Corentin Labbe <clabbe.montjoie@...il.com>,
        Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
        Jason Gunthorpe <jgunthorpe@...idianresearch.com>,
        Jerry Snitselaar <jsnitsel@...hat.com>,
        Kenneth Goldman <kgold@...ux.vnet.ibm.com>,
        Michael Ellerman <mpe@...erman.id.au>,
        Nayna Jain <nayna@...ux.vnet.ibm.com>,
        Paul Mackerras <paulus@...ba.org>,
        Peter Hüwe <PeterHuewe@....de>,
        Stefan Berger <stefanb@...ux.vnet.ibm.com>,
        kernel-janitors@...r.kernel.org,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/4] char/tpm: Less checks in tpm_ibmvtpm_probe() after
 error detection

Hello,


On Mon, 16 Oct 2017 19:34:56 +0200
SF Markus Elfring <elfring@...rs.sourceforge.net> wrote:

> From: Markus Elfring <elfring@...rs.sourceforge.net>
> Date: Mon, 16 Oct 2017 19:00:34 +0200
> 
> Two pointer checks could be repeated by the tpm_ibmvtpm_probe()
> function during error handling even if the relevant properties can be
> determined for the involved variables before by source code analysis.
> 
> * Return directly after a call of the function "kzalloc" failed
>   at the beginning.
> 
> * Adjust jump targets so that extra checks can be omitted at the end.
> 
> Signed-off-by: Markus Elfring <elfring@...rs.sourceforge.net>
> ---
>  drivers/char/tpm/tpm_ibmvtpm.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_ibmvtpm.c
> b/drivers/char/tpm/tpm_ibmvtpm.c index a4b462a77b99..b8dda7546f64
> 100644 --- a/drivers/char/tpm/tpm_ibmvtpm.c
> +++ b/drivers/char/tpm/tpm_ibmvtpm.c
> @@ -610,7 +610,7 @@ static int tpm_ibmvtpm_probe(struct vio_dev
> *vio_dev, 
>  	ibmvtpm = kzalloc(sizeof(*ibmvtpm), GFP_KERNEL);
>  	if (!ibmvtpm)
> -		goto cleanup;
> +		return -ENOMEM;

Just no.

I have seen many fixes that do inverse of this after a piece of code
allocating some more resources was added before code that returns
straight away because it is the first allocation in a function.

>  
>  	ibmvtpm->dev = dev;
>  	ibmvtpm->vdev = vio_dev;
> @@ -619,7 +619,7 @@ static int tpm_ibmvtpm_probe(struct vio_dev
> *vio_dev, crq_q->crq_addr = (struct ibmvtpm_crq
> *)get_zeroed_page(GFP_KERNEL); if (!crq_q->crq_addr) {
>  		dev_err(dev, "Unable to allocate memory for
> crq_addr\n");
> -		goto cleanup;
> +		goto free_tpm;
>  	}
>  
>  	crq_q->num_entry = CRQ_RES_BUF_SIZE /
> sizeof(*crq_q->crq_addr); @@ -629,7 +629,7 @@ static int
> tpm_ibmvtpm_probe(struct vio_dev *vio_dev, 
>  	if (dma_mapping_error(dev, ibmvtpm->crq_dma_handle)) {
>  		dev_err(dev, "dma mapping failed\n");
> -		goto cleanup;
> +		goto free_page;
>  	}
>  
>  	rc = plpar_hcall_norets(H_REG_CRQ, vio_dev->unit_address,
> @@ -683,13 +683,10 @@ static int tpm_ibmvtpm_probe(struct vio_dev
> *vio_dev, reg_crq_cleanup:
>  	dma_unmap_single(dev, ibmvtpm->crq_dma_handle,
> CRQ_RES_BUF_SIZE, DMA_BIDIRECTIONAL);
> -cleanup:
> -	if (ibmvtpm) {
> -		if (crq_q->crq_addr)
> -			free_page((unsigned long)crq_q->crq_addr);
> -		kfree(ibmvtpm);
> -	}
> -

I think a single cleanup section is better than many labels that just
avoid a single null check.

As long as you can tell easily which resources were already allocated
and need to be freed it is saner to keep only one cleanup section.

If the code doing the allocation is changed in the future the single
cleanup can stay whereas multiple labels have to be rewritten again.

Also just changing this just for the sake of code style does not seem
worth it whatever style you prefer.

Thanks

Michal