From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
To: "Winkler, Tomas" <tomas.winkler@intel.com>
Cc: "'Jason Gunthorpe'" <jgg@ziepe.ca>,
"Usyskin, Alexander" <alexander.usyskin@intel.com>,
"'linux-integrity@vger.kernel.org'"
<linux-integrity@vger.kernel.org>,
"'linux-security-module@vger.kernel.org'"
<linux-security-module@vger.kernel.org>,
"'linux-kernel@vger.kernel.org'" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] tpm: tpm_crb: relinquish locality on error path.
Date: Tue, 24 Apr 2018 20:48:02 +0300 [thread overview]
Message-ID: <20180424174802.GA24613@linux.intel.com> (raw)
In-Reply-To: <20180424160328.GA5119@linux.intel.com>
On Tue, Apr 24, 2018 at 07:03:28PM +0300, Jarkko Sakkinen wrote:
> On Fri, Apr 20, 2018 at 01:19:12PM +0000, Winkler, Tomas wrote:
> > > > On Tue, 2018-04-10 at 09:00 +0000, Winkler, Tomas wrote:
> > > > > >
> > > > > > On Sat, 2018-04-07 at 19:12 +0300, Tomas Winkler wrote:
> > > > > > > In crb_map_io() function, __crb_request_locality() is called
> > > > > > > prior to crb_cmd_ready(), but if one of the consecutive function
> > > > > > > fails the flow bails out instead of trying to relinquish locality.
> > > > > > > This patch adds goto jump to __crb_relinquish_locality() on the
> > > > > > > error path.
> > > > > > >
> > > > > > > Fixes: 888d867df441 (tpm: cmd_ready command can be issued only
> > > > > > > after granting
> > > > > > > locality)
> > > > > > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > > > > > > ---
> > > > > > > drivers/char/tpm/tpm_crb.c | 10 +++++++---
> > > > > > > 1 file changed, 7 insertions(+), 3 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/char/tpm/tpm_crb.c
> > > > > > > b/drivers/char/tpm/tpm_crb.c index 7f78482cd157..34fbc6cb097b
> > > > > > > 100644
> > > > > > > --- a/drivers/char/tpm/tpm_crb.c
> > > > > > > +++ b/drivers/char/tpm/tpm_crb.c
> > > > > > > @@ -511,8 +511,10 @@ static int crb_map_io(struct acpi_device
> > > > > > > *device, struct crb_priv *priv,
> > > > > > >
> > > > > > > priv->regs_t = crb_map_res(dev, priv, &io_res, buf-
> > > > > > > >control_address,
> > > > > > > sizeof(struct crb_regs_tail));
> > > > > > > - if (IS_ERR(priv->regs_t))
> > > > > > > - return PTR_ERR(priv->regs_t);
> > > > > > > + if (IS_ERR(priv->regs_t)) {
> > > > > > > + ret = PTR_ERR(priv->regs_t);
> > > > > > > + goto out_relinquish_locality;
> > > > > > > + }
> > > > > > >
> > > > > > > /*
> > > > > > > * PTT HW bug w/a: wake up the device to access @@ -520,7
> > > > > > > +522,7
> > > > > >
> > > > > > @@
> > > > > > > static int crb_map_io(struct acpi_device *device, struct
> > > > > > > crb_priv *priv,
> > > > > > > */
> > > > > > > ret = crb_cmd_ready(dev, priv);
> > > > > > > if (ret)
> > > > > > > - return ret;
> > > > > > > + goto out_relinquish_locality;
> > > > > > >
> > > > > > > pa_high = ioread32(&priv->regs_t->ctrl_cmd_pa_high);
> > > > > > > pa_low = ioread32(&priv->regs_t->ctrl_cmd_pa_low);
> > > > > > > @@ -565,6 +567,8 @@ static int crb_map_io(struct acpi_device
> > > > > > > *device, struct crb_priv *priv,
> > > > > > >
> > > > > > > crb_go_idle(dev, priv);
> > > > > > >
> > > > > > > +out_relinquish_locality:
> > > > > > > +
> > > > > > > __crb_relinquish_locality(dev, priv, 0);
> > > > > > >
> > > > > > > return ret;
> > > > > >
> > > > > > Thanks, please just call it before returning in the error path.
> > > > >
> > > > > Can you please elaborate why, isn't the centralized exiting of
> > > > > functions preferred kernel coding style?
> > > > > https://www.kernel.org/doc/html/v4.11/process/coding-style.html#cent
> > > > > ra
> > > > > lized-ex
> > > > > iting-of-functions
> > > >
> > > > You exit only from one location (not multiple) and not from a nested
> > > > context. Here you just add more complexity by doing this.
> > >
> > > Where is the complexity ? I see it as a standard way of undoing on exit.
> > > Tomas
> >
> > Jarkko, can you please respond.
> > Thanks
> > Tomas
>
> I was away for Mon-Wed last week and did not work on TPM for Thu-Fri.
>
> My earlier comment was incorrect as there are two locations to exit (not
> sure how I managed to overlook the patch that way).
>
> Thus, I have only two very minor requets:
>
> * Remove the extra newline (the last line addition in the patch).
> * Use just label named out as we have only one exception handler.
>
> I'll move on to testing, and if it it passes, I can do those updates
> myself.
>
> /Jarkko
Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
/Jarkko
WARNING: multiple messages have this Message-ID (diff)
From: jarkko.sakkinen@linux.intel.com (Jarkko Sakkinen)
To: linux-security-module@vger.kernel.org
Subject: [PATCH] tpm: tpm_crb: relinquish locality on error path.
Date: Tue, 24 Apr 2018 20:48:02 +0300 [thread overview]
Message-ID: <20180424174802.GA24613@linux.intel.com> (raw)
In-Reply-To: <20180424160328.GA5119@linux.intel.com>
On Tue, Apr 24, 2018 at 07:03:28PM +0300, Jarkko Sakkinen wrote:
> On Fri, Apr 20, 2018 at 01:19:12PM +0000, Winkler, Tomas wrote:
> > > > On Tue, 2018-04-10 at 09:00 +0000, Winkler, Tomas wrote:
> > > > > >
> > > > > > On Sat, 2018-04-07 at 19:12 +0300, Tomas Winkler wrote:
> > > > > > > In crb_map_io() function, __crb_request_locality() is called
> > > > > > > prior to crb_cmd_ready(), but if one of the consecutive function
> > > > > > > fails the flow bails out instead of trying to relinquish locality.
> > > > > > > This patch adds goto jump to __crb_relinquish_locality() on the
> > > > > > > error path.
> > > > > > >
> > > > > > > Fixes: 888d867df441 (tpm: cmd_ready command can be issued only
> > > > > > > after granting
> > > > > > > locality)
> > > > > > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > > > > > > ---
> > > > > > > drivers/char/tpm/tpm_crb.c | 10 +++++++---
> > > > > > > 1 file changed, 7 insertions(+), 3 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/char/tpm/tpm_crb.c
> > > > > > > b/drivers/char/tpm/tpm_crb.c index 7f78482cd157..34fbc6cb097b
> > > > > > > 100644
> > > > > > > --- a/drivers/char/tpm/tpm_crb.c
> > > > > > > +++ b/drivers/char/tpm/tpm_crb.c
> > > > > > > @@ -511,8 +511,10 @@ static int crb_map_io(struct acpi_device
> > > > > > > *device, struct crb_priv *priv,
> > > > > > >
> > > > > > > priv->regs_t = crb_map_res(dev, priv, &io_res, buf-
> > > > > > > >control_address,
> > > > > > > sizeof(struct crb_regs_tail));
> > > > > > > - if (IS_ERR(priv->regs_t))
> > > > > > > - return PTR_ERR(priv->regs_t);
> > > > > > > + if (IS_ERR(priv->regs_t)) {
> > > > > > > + ret = PTR_ERR(priv->regs_t);
> > > > > > > + goto out_relinquish_locality;
> > > > > > > + }
> > > > > > >
> > > > > > > /*
> > > > > > > * PTT HW bug w/a: wake up the device to access @@ -520,7
> > > > > > > +522,7
> > > > > >
> > > > > > @@
> > > > > > > static int crb_map_io(struct acpi_device *device, struct
> > > > > > > crb_priv *priv,
> > > > > > > */
> > > > > > > ret = crb_cmd_ready(dev, priv);
> > > > > > > if (ret)
> > > > > > > - return ret;
> > > > > > > + goto out_relinquish_locality;
> > > > > > >
> > > > > > > pa_high = ioread32(&priv->regs_t->ctrl_cmd_pa_high);
> > > > > > > pa_low = ioread32(&priv->regs_t->ctrl_cmd_pa_low);
> > > > > > > @@ -565,6 +567,8 @@ static int crb_map_io(struct acpi_device
> > > > > > > *device, struct crb_priv *priv,
> > > > > > >
> > > > > > > crb_go_idle(dev, priv);
> > > > > > >
> > > > > > > +out_relinquish_locality:
> > > > > > > +
> > > > > > > __crb_relinquish_locality(dev, priv, 0);
> > > > > > >
> > > > > > > return ret;
> > > > > >
> > > > > > Thanks, please just call it before returning in the error path.
> > > > >
> > > > > Can you please elaborate why, isn't the centralized exiting of
> > > > > functions preferred kernel coding style?
> > > > > https://www.kernel.org/doc/html/v4.11/process/coding-style.html#cent
> > > > > ra
> > > > > lized-ex
> > > > > iting-of-functions
> > > >
> > > > You exit only from one location (not multiple) and not from a nested
> > > > context. Here you just add more complexity by doing this.
> > >
> > > Where is the complexity ? I see it as a standard way of undoing on exit.
> > > Tomas
> >
> > Jarkko, can you please respond.
> > Thanks
> > Tomas
>
> I was away for Mon-Wed last week and did not work on TPM for Thu-Fri.
>
> My earlier comment was incorrect as there are two locations to exit (not
> sure how I managed to overlook the patch that way).
>
> Thus, I have only two very minor requets:
>
> * Remove the extra newline (the last line addition in the patch).
> * Use just label named out as we have only one exception handler.
>
> I'll move on to testing, and if it it passes, I can do those updates
> myself.
>
> /Jarkko
Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2018-04-24 17:48 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-04-07 16:12 [PATCH] tpm: tpm_crb: relinquish locality on error path Tomas Winkler
2018-04-07 16:12 ` Tomas Winkler
2018-04-09 11:13 ` Jarkko Sakkinen
2018-04-09 11:13 ` Jarkko Sakkinen
2018-04-10 9:00 ` Winkler, Tomas
2018-04-10 9:00 ` Winkler, Tomas
2018-04-10 12:33 ` Jarkko Sakkinen
2018-04-10 12:33 ` Jarkko Sakkinen
2018-04-10 20:25 ` Winkler, Tomas
2018-04-10 20:25 ` Winkler, Tomas
2018-04-20 13:19 ` Winkler, Tomas
2018-04-20 13:19 ` Winkler, Tomas
2018-04-24 16:03 ` Jarkko Sakkinen
2018-04-24 16:03 ` Jarkko Sakkinen
2018-04-24 17:48 ` Jarkko Sakkinen [this message]
2018-04-24 17:48 ` Jarkko Sakkinen
2018-04-24 20:10 ` Winkler, Tomas
2018-04-24 20:10 ` Winkler, Tomas
2018-04-24 20:04 ` Winkler, Tomas
2018-04-24 20:04 ` Winkler, Tomas
2018-05-02 13:32 ` Jarkko Sakkinen
2018-05-02 13:32 ` Jarkko Sakkinen
2018-05-02 13:35 ` Winkler, Tomas
2018-05-02 13:35 ` Winkler, Tomas
2018-05-03 6:35 ` Jarkko Sakkinen
2018-05-03 6:35 ` Jarkko Sakkinen
2018-05-03 6:42 ` Winkler, Tomas
2018-05-03 6:42 ` Winkler, Tomas
2018-05-04 8:16 ` Jarkko Sakkinen
2018-05-04 8:16 ` Jarkko Sakkinen
2018-05-05 21:07 ` Winkler, Tomas
2018-05-05 21:07 ` Winkler, Tomas
2018-05-09 12:26 ` Jarkko Sakkinen
2018-05-09 12:26 ` Jarkko Sakkinen
2018-05-09 21:49 ` Winkler, Tomas
2018-05-09 21:49 ` Winkler, Tomas
2018-05-11 10:18 ` Jarkko Sakkinen
2018-05-11 10:18 ` Jarkko Sakkinen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180424174802.GA24613@linux.intel.com \
--to=jarkko.sakkinen@linux.intel.com \
--cc=alexander.usyskin@intel.com \
--cc=jgg@ziepe.ca \
--cc=linux-integrity@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=tomas.winkler@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.