[Qemu-devel] [PATCH 0/2] sd: sdhci: correct transfer mode register usage

[Qemu-devel] [PATCH 0/2] sd: sdhci: correct transfer mode register usage

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

Hello,

In SDHCI emulation, the 'Block Count Enable' bit of the Transfer Mode
register is used to control 's->blkcnt' value. One, this bit is not
relevant in single block transfers. Second, Transfer Mode register
value could be set such that 's->blkcnt' would not see an update
during multi block transfers. Thus leading to an infinite loop.

This patch set attempts to correct 'Block Count Enable' bit usage.

Thank you.
--
Prasad J Pandit (2):
  sd: sdhci: check transfer mode register in multi block transfer
  sd: sdhci: block count enable not relevant in single block transfer

 hw/sd/sdhci.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

--
2.9.3

[Qemu-devel] [PATCH 1/2] sd: sdhci: check transfer mode register in multi block transfer

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

In SDHCI device emulation the transfer mode register value
is used during multi block transfer to check if block count
register is enabled and should be updated. Transfer mode
register could be set such that, block count register would
not be updated, thus leading to an infinite loop. Add check
to avoid it.

Reported-by: Wjjzhang <wjjzhang@tencent.com>
Reported-by: Jiang Xin <jiangxin1@huawei.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/sd/sdhci.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 5bd5ab6..150464f 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -486,6 +486,12 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
     uint32_t boundary_chk = 1 << (((s->blksize & 0xf000) >> 12) + 12);
     uint32_t boundary_count = boundary_chk - (s->sdmasysad % boundary_chk);
 
+    if (!(s->trnmod & SDHC_TRNS_MULTI)
+        || !(s->trnmod & SDHC_TRNS_BLK_CNT_EN)
+        || !s->blkcnt) {
+        return;
+    }
+
     /* XXX: Some sd/mmc drivers (for example, u-boot-slp) do not account for
      * possible stop at page boundary if initial address is not page aligned,
      * allow them to work properly */
@@ -797,11 +803,6 @@ static void sdhci_data_transfer(void *opaque)
     if (s->trnmod & SDHC_TRNS_DMA) {
         switch (SDHC_DMA_TYPE(s->hostctl)) {
         case SDHC_CTRL_SDMA:
-            if ((s->trnmod & SDHC_TRNS_MULTI) &&
-                    (!(s->trnmod & SDHC_TRNS_BLK_CNT_EN) || s->blkcnt == 0)) {
-                break;
-            }
-
             if ((s->blkcnt == 1) || !(s->trnmod & SDHC_TRNS_MULTI)) {
                 sdhci_sdma_transfer_single_block(s);
             } else {
@@ -1050,7 +1051,7 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
         if (!(s->capareg & SDHC_CAN_DO_DMA)) {
             value &= ~SDHC_TRNS_DMA;
         }
-        MASKED_WRITE(s->trnmod, mask, value);
+        MASKED_WRITE(s->trnmod, mask, value & 0x0037);
         MASKED_WRITE(s->cmdreg, mask >> 16, value >> 16);
 
         /* Writing to the upper byte of CMDREG triggers SD command generation */
-- 
2.9.3

[Qemu-devel] [PATCH 2/2] sd: sdhci: block count enable not relevant in single block transfer

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

In SDHCI device emulation the 'Block count enable' bit
of the Transfer Mode register is only relevant in multi block
transfers. We need not check it in single block transfers.

Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/sd/sdhci.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 150464f..d921423 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -570,7 +570,6 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
 }
 
 /* single block SDMA transfer */
-
 static void sdhci_sdma_transfer_single_block(SDHCIState *s)
 {
     int n;
@@ -589,10 +588,7 @@ static void sdhci_sdma_transfer_single_block(SDHCIState *s)
             sdbus_write_data(&s->sdbus, s->fifo_buffer[n]);
         }
     }
-
-    if (s->trnmod & SDHC_TRNS_BLK_CNT_EN) {
-        s->blkcnt--;
-    }
+    s->blkcnt--;
 
     sdhci_end_transfer(s);
 }
-- 
2.9.3

Re: [Qemu-devel] [PATCH 0/2] sd: sdhci: correct transfer mode register usage

[P J P]

+-- On Tue, 31 Jan 2017, P J P wrote --+
| In SDHCI emulation, the 'Block Count Enable' bit of the Transfer Mode
| register is used to control 's->blkcnt' value. One, this bit is not
| relevant in single block transfers. Second, Transfer Mode register
| value could be set such that 's->blkcnt' would not see an update
| during multi block transfers. Thus leading to an infinite loop.
| 
| This patch set attempts to correct 'Block Count Enable' bit usage.
| 
| Thank you.
| --
| Prasad J Pandit (2):
|   sd: sdhci: check transfer mode register in multi block transfer
|   sd: sdhci: block count enable not relevant in single block transfer

Ping...!
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH 0/2] sd: sdhci: correct transfer mode register usage

[Peter Maydell]

On 31 January 2017 at 12:24, P J P <ppandit@redhat.com> wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> Hello,
>
> In SDHCI emulation, the 'Block Count Enable' bit of the Transfer Mode
> register is used to control 's->blkcnt' value. One, this bit is not
> relevant in single block transfers. Second, Transfer Mode register
> value could be set such that 's->blkcnt' would not see an update
> during multi block transfers. Thus leading to an infinite loop.
>
> This patch set attempts to correct 'Block Count Enable' bit usage.

Edgar, Alistair: the zynq models are our major SDHCI user -- would
you like to have a look at this patchset, please?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/2] sd: sdhci: correct transfer mode register usage

[P J P]

+-- On Tue, 7 Feb 2017, Peter Maydell wrote --+
| On 31 January 2017 at 12:24, P J P <ppandit@redhat.com> wrote:
| > In SDHCI emulation, the 'Block Count Enable' bit of the Transfer Mode
| > register is used to control 's->blkcnt' value. One, this bit is not
| > relevant in single block transfers. Second, Transfer Mode register
| > value could be set such that 's->blkcnt' would not see an update
| > during multi block transfers. Thus leading to an infinite loop.
| >
| > This patch set attempts to correct 'Block Count Enable' bit usage.
| 
| Edgar, Alistair: the zynq models are our major SDHCI user -- would
| you like to have a look at this patchset, please?

I suspect following patch would also be required along with the two in this 
series, not sure.

===
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index d921423..7f3d547 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1019,7 +1019,11 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, 
unsigned size)
         /* Writing to last byte of sdmasysad might trigger transfer */
         if (!(mask & 0xFF000000) && TRANSFERRING_DATA(s->prnsts) && s->blkcnt 
&&
                 s->blksize && SDHC_DMA_TYPE(s->hostctl) == SDHC_CTRL_SDMA) {
-            sdhci_sdma_transfer_multi_blocks(s);
+            if (!(s->trnmod & SDHC_TRNS_MULTI)) {
+                sdhci_sdma_transfer_single_block(s);
+            } else {
+                sdhci_sdma_transfer_multi_blocks(s);
+            }
         }
         break;
     case SDHC_BLKSIZE:
===

Could you please have a look this one too?

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH 0/2] sd: sdhci: correct transfer mode register usage

[Alistair Francis]

On Tue, Feb 7, 2017 at 11:12 AM, P J P <ppandit@redhat.com> wrote:
> +-- On Tue, 7 Feb 2017, Peter Maydell wrote --+
> | On 31 January 2017 at 12:24, P J P <ppandit@redhat.com> wrote:
> | > In SDHCI emulation, the 'Block Count Enable' bit of the Transfer Mode
> | > register is used to control 's->blkcnt' value. One, this bit is not
> | > relevant in single block transfers. Second, Transfer Mode register
> | > value could be set such that 's->blkcnt' would not see an update
> | > during multi block transfers. Thus leading to an infinite loop.
> | >
> | > This patch set attempts to correct 'Block Count Enable' bit usage.
> |
> | Edgar, Alistair: the zynq models are our major SDHCI user -- would
> | you like to have a look at this patchset, please?

Yeah, I'll have a look.

>
> I suspect following patch would also be required along with the two in this
> series, not sure.
>
> ===
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index d921423..7f3d547 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1019,7 +1019,11 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val,
> unsigned size)
>          /* Writing to last byte of sdmasysad might trigger transfer */
>          if (!(mask & 0xFF000000) && TRANSFERRING_DATA(s->prnsts) && s->blkcnt
> &&
>                  s->blksize && SDHC_DMA_TYPE(s->hostctl) == SDHC_CTRL_SDMA) {
> -            sdhci_sdma_transfer_multi_blocks(s);
> +            if (!(s->trnmod & SDHC_TRNS_MULTI)) {
> +                sdhci_sdma_transfer_single_block(s);
> +            } else {
> +                sdhci_sdma_transfer_multi_blocks(s);
> +            }
>          }
>          break;
>      case SDHC_BLKSIZE:
> ===
>
> Could you please have a look this one too?

Sorry I'm confused. Should this be a third patch or is this in a
different series?

Thanks,

Alistair

>
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
>

Re: [Qemu-devel] [PATCH 1/2] sd: sdhci: check transfer mode register in multi block transfer

[Alistair Francis]

On Tue, Jan 31, 2017 at 4:24 AM, P J P <ppandit@redhat.com> wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> In SDHCI device emulation the transfer mode register value
> is used during multi block transfer to check if block count
> register is enabled and should be updated. Transfer mode
> register could be set such that, block count register would
> not be updated, thus leading to an infinite loop. Add check
> to avoid it.
>
> Reported-by: Wjjzhang <wjjzhang@tencent.com>
> Reported-by: Jiang Xin <jiangxin1@huawei.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/sd/sdhci.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 5bd5ab6..150464f 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -486,6 +486,12 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
>      uint32_t boundary_chk = 1 << (((s->blksize & 0xf000) >> 12) + 12);
>      uint32_t boundary_count = boundary_chk - (s->sdmasysad % boundary_chk);
>
> +    if (!(s->trnmod & SDHC_TRNS_MULTI)
> +        || !(s->trnmod & SDHC_TRNS_BLK_CNT_EN)
> +        || !s->blkcnt) {
> +        return;
> +    }
> +

This doesn't seem completely right to me.

Looking at the SD spec (pg.39 so I can find it in future) the block
count enable bit does only apply to multiple block transfers. So
moving it into the multi block transfer function looks correct (good
catch).

The spec says that the block count enable bit can be set to zero and
that disables the block count register, which is useful for an
infinite transfer. The bit should also be set to 0 if the data
transfer is more then 65535 blocks.

As it doesn't look like we have ever supported an infinite transfer
and we don't want infinite loops occurring I think we should add an
unsupported print statement here saying that we don't support infinite
transfers and then I'm ok with this patch.

>      /* XXX: Some sd/mmc drivers (for example, u-boot-slp) do not account for
>       * possible stop at page boundary if initial address is not page aligned,
>       * allow them to work properly */
> @@ -797,11 +803,6 @@ static void sdhci_data_transfer(void *opaque)
>      if (s->trnmod & SDHC_TRNS_DMA) {
>          switch (SDHC_DMA_TYPE(s->hostctl)) {
>          case SDHC_CTRL_SDMA:
> -            if ((s->trnmod & SDHC_TRNS_MULTI) &&
> -                    (!(s->trnmod & SDHC_TRNS_BLK_CNT_EN) || s->blkcnt == 0)) {
> -                break;
> -            }
> -
>              if ((s->blkcnt == 1) || !(s->trnmod & SDHC_TRNS_MULTI)) {
>                  sdhci_sdma_transfer_single_block(s);
>              } else {
> @@ -1050,7 +1051,7 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
>          if (!(s->capareg & SDHC_CAN_DO_DMA)) {
>              value &= ~SDHC_TRNS_DMA;
>          }
> -        MASKED_WRITE(s->trnmod, mask, value);
> +        MASKED_WRITE(s->trnmod, mask, value & 0x0037);

What is this change doing?

Thanks,

Alistair

>          MASKED_WRITE(s->cmdreg, mask >> 16, value >> 16);
>
>          /* Writing to the upper byte of CMDREG triggers SD command generation */
> --
> 2.9.3
>
>

Re: [Qemu-devel] [PATCH 2/2] sd: sdhci: block count enable not relevant in single block transfer

[Alistair Francis]

On Tue, Jan 31, 2017 at 4:24 AM, P J P <ppandit@redhat.com> wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> In SDHCI device emulation the 'Block count enable' bit

Can you say 'In the SDHCI protocol' instead of 'SDHCI device emulation'?

I feel like saying device emulation makes it sound like we are
behaving different then hardware.

Also maybe the title should be something more like:
sd: sdhci: Remove block count enable check in single block transfers
to make it clearer.

> of the Transfer Mode register is only relevant in multi block
> transfers. We need not check it in single block transfers.
>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>

The functionality looks good to me.

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,

Alistair

> ---
>  hw/sd/sdhci.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 150464f..d921423 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -570,7 +570,6 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
>  }
>
>  /* single block SDMA transfer */
> -
>  static void sdhci_sdma_transfer_single_block(SDHCIState *s)
>  {
>      int n;
> @@ -589,10 +588,7 @@ static void sdhci_sdma_transfer_single_block(SDHCIState *s)
>              sdbus_write_data(&s->sdbus, s->fifo_buffer[n]);
>          }
>      }
> -
> -    if (s->trnmod & SDHC_TRNS_BLK_CNT_EN) {
> -        s->blkcnt--;
> -    }
> +    s->blkcnt--;
>
>      sdhci_end_transfer(s);
>  }
> --
> 2.9.3
>
>

Re: [Qemu-devel] [PATCH 0/2] sd: sdhci: correct transfer mode register usage

[P J P]

+-- On Tue, 7 Feb 2017, Alistair Francis wrote --+
| > ===
| > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
| > index d921423..7f3d547 100644
| > --- a/hw/sd/sdhci.c
| > +++ b/hw/sd/sdhci.c
| > @@ -1019,7 +1019,11 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val,
| > unsigned size)
| >          /* Writing to last byte of sdmasysad might trigger transfer */
| >          if (!(mask & 0xFF000000) && TRANSFERRING_DATA(s->prnsts) && s->blkcnt
| > &&
| >                  s->blksize && SDHC_DMA_TYPE(s->hostctl) == SDHC_CTRL_SDMA) {
| > -            sdhci_sdma_transfer_multi_blocks(s);
| > +            if (!(s->trnmod & SDHC_TRNS_MULTI)) {
| > +                sdhci_sdma_transfer_single_block(s);
| > +            } else {
| > +                sdhci_sdma_transfer_multi_blocks(s);
| > +            }
| >          }
| >          break;
| >      case SDHC_BLKSIZE:
| > ===
| 
| Should this be a third patch or is this in a different series?

  Yes, a third patch in the series; If it is required.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH 1/2] sd: sdhci: check transfer mode register in multi block transfer

[P J P]

  Hello Alistair,

+-- On Tue, 7 Feb 2017, Alistair Francis wrote --+
| As it doesn't look like we have ever supported an infinite transfer
| and we don't want infinite loops occurring I think we should add an
| unsupported print statement here saying that we don't support infinite
| transfers and then I'm ok with this patch.

  Okay, will do.
 
| > -        MASKED_WRITE(s->trnmod, mask, value);
| > +        MASKED_WRITE(s->trnmod, mask, value & 0x0037);
| 
| What is this change doing?

  It is possible to set 's->trnmod' to an invalid value(> 6 bits). Above 
mask(0x0037) would limit 'value' to the 6 bits mentioned in the spec.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F