qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Stefan Berger <stefanb@linux.ibm.com>
To: P J P <ppandit@redhat.com>, QEMU Developers <qemu-devel@nongnu.org>
Cc: "Marc-André Lureau" <marcandre.lureau@gmail.com>,
	"Stefan Berger" <stefanb@linux.vnet.ibm.com>,
	"Cheng Feng" <PSIRT@huawei.com>,
	"Prasad J Pandit" <pjp@fedoraproject.org>
Subject: Re: [Qemu-devel] [PATCH v1] tpm: check localities index
Date: Wed, 21 Nov 2018 10:41:32 -0500	[thread overview]
Message-ID: <5fcd30a0-f932-3f06-61a5-45ddc72ce606@linux.ibm.com> (raw)
In-Reply-To: <20181120072239.512-1-ppandit@redhat.com>

On 11/20/18 2:22 AM, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> While performing mmio device r/w operations, guest could set 'addr'
> parameter such that 'locty' index exceeds TPM_TIS_NUM_LOCALITIES=5
> after setting new 'locty' via 'tpm_tis_new_active_locality'.
> Add check to avoid OOB access.
>
> Reported-by: Cheng Feng <PSIRT@huawei.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>   hw/tpm/tpm_tis.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> Update: add assert() calls
>    -> https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg00912.html
>
> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> index 12f5c9a759..d6bf3ceb26 100644
> --- a/hw/tpm/tpm_tis.c
> +++ b/hw/tpm/tpm_tis.c
> @@ -293,6 +293,7 @@ static void tpm_tis_request_completed(TPMIf *ti, int ret)
>       uint8_t locty = s->cmd.locty;
>       uint8_t l;
>
> +    assert(TPM_TIS_IS_VALID_LOCTY(locty));


This one I may take. However, the tpm_tis_tpm_send is called with a 
valid locality (derived from the MMIO address) and passes the locality 
number to the backend. The backends are reading the locality but don't 
change it.


>       if (s->cmd.selftest_done) {
>           for (l = 0; l < TPM_TIS_NUM_LOCALITIES; l++) {
>               s->loc[locty].sts |= TPM_TIS_STS_SELFTEST_DONE;
> @@ -401,6 +402,7 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr addr,
>       uint32_t avail;
>       uint8_t v;
>
> +    assert(TPM_TIS_IS_VALID_LOCTY(locty));


This check comes right after

  uint8_t locty = tpm_tis_locality_from_addr(addr);

which gives us a locty in range [0.. 4]. So I don't think a check here 
is needed.


>       if (tpm_backend_had_startup_error(s->be_driver)) {
>           return 0;
>       }
> @@ -523,6 +525,7 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
>       uint16_t len;
>       uint32_t mask = (size == 1) ? 0xff : ((size == 2) ? 0xffff : ~0);
>
> +    assert(TPM_TIS_IS_VALID_LOCTY(locty));


Same comment here.

>       trace_tpm_tis_mmio_write(size, addr, val);
>
>       if (locty == 4) {
> @@ -642,7 +645,7 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
>               }
>           }
>
> -        if (set_new_locty) {
> +        if (set_new_locty && TPM_TIS_IS_VALID_LOCTY(active_locty)) {

It is possible that no locality is active by the current locality 
relinquishing the ownership and we have to be able to set that.


>               tpm_tis_new_active_locality(s, active_locty);
>           }
>

  parent reply	other threads:[~2018-11-21 15:41 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-20  7:22 [Qemu-devel] [PATCH v1] tpm: check localities index P J P
2018-11-20  8:02 ` Marc-André Lureau
2018-11-20 11:23   ` Marc-André Lureau
2018-11-20 15:52 ` Stefan Berger
2018-11-20 18:06   ` P J P
2018-11-21 11:52     ` P J P
2018-11-21 13:36     ` Stefan Berger
2018-11-22 10:11       ` P J P
2018-11-21 15:41 ` Stefan Berger [this message]
2018-11-21 16:26 ` no-reply
2018-11-21 17:54   ` Stefan Berger

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=5fcd30a0-f932-3f06-61a5-45ddc72ce606@linux.ibm.com \
    --to=stefanb@linux.ibm.com \
    --cc=PSIRT@huawei.com \
    --cc=marcandre.lureau@gmail.com \
    --cc=pjp@fedoraproject.org \
    --cc=ppandit@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=stefanb@linux.vnet.ibm.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).