linux-kernel - Re: [PATCH] vTPM: fix missing error handling for suspend operation

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: <1457477678.800.7.camel@vtpm2014.ibm.com>
Date:	Tue, 08 Mar 2016 17:54:38 -0500
From:	"Hon Ching(Vicky) Lo" <honclo@...ux.vnet.ibm.com>
To:	Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
Cc:	tpmdd-devel@...ts.sourceforge.net, Peter Huewe <PeterHuewe@....de>,
	Ashley Lai <ashley@...leylai.com>,
	Vicky Lo <honclo2014@...il.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] vTPM: fix missing error handling for suspend operation

> > > +		} else if (crq.msg == (u8) PARTNER_PARTITION_FAILED) {
> > > +			dev_err(ibmvtpm->dev,
> > > +				"vtpm has terminated fatally; reboot to reinstate a trusted state.\n");
> > > +		} else if (crq.msg == (u8) PARTNER_PARTITION_DEREG_CRQ) {
> > > +			/* The vtpm is in the process of being reloaded by
> > > +			 * firmware and has de-registered CRQ.  The client
> > > +			 * must wait for the CRQ INITIALIZATION message and
> > > +			 * respond and must resubmit suspend message.
> > > +			 */
> > > +			sig =
> > > +			    wait_event_interruptible(ibmvtpm->wq,
> > > +						     crq_initialized == 1);
> > > +			if (sig)
> > > +				return -EINTR;
> > > +
> > > +			if (suspend_again_count < 1) {
> > > +				suspend_again_count++;
> > > +				goto suspendagain;
> > > +			}
> > > +		} else
> > > +			;
> > > +	}
> > > +
> > >  	if (rc != H_SUCCESS)
> > > -		dev_err(ibmvtpm->dev,
> > > -			"tpm_ibmvtpm_suspend failed rc=%d\n", rc);
> > > +		dev_err(ibmvtpm->dev, "tpm_ibmvtpm_suspend failed rc=%d\n", rc);
> > >  
> > >  	return rc;
> > > +
> > > +suspendagain:
> > > +	rc = tpm_ibmvtpm_suspend(ibmvtpm->dev);
> > > +	suspend_again_count = 0;
> > > +
> > > +	if (rc != H_SUCCESS)
> > > +		dev_err(ibmvtpm->dev, "tpm_ibmvtpm_suspend failed rc=%d\n", rc);
> > > +
> > > +	return rc;
> > > +
> > 
> > Get rid of this horrible looking tail recursion thing.
> > 
> > What the heck is suspend_again_count and why it can be module scope
> > variable? You could use a local variable instead if you would iterate
> > with a loop.
> > 
> > /Jarkko
> > 
> 
> The reason for the 'goto' statement and the suspend_again_count was to
> prevent the suspend function recurse again.  In the case if vtpm is in
> the process of being reloaded by firmware, we want to wait for the CRQ
> INITIALIZATION and resubmit suspend message i.e. recurse only once.
> 
Never mind..  I don't really save any repetitive code by using recursion
now.  I'll rework and resubmit the patch.


Thanks,
Vicky