Re: [Qemu-devel] [PATCH v1 1/1] sdhci.c: Limit the maximum block size

[Stefan Hajnoczi]

On Tue, Oct 06, 2015 at 10:40:41AM -0700, Alistair Francis wrote:
> It is possible for the guest to set an invalid block
> size which is larger then the fifo_buffer[] array. This
> could cause a buffer overflow.
> 
> To avoid this limit the maximum size of the blksize variable.
> 
> Signed-off-by: Alistair Francis 
> Suggested-by: Igor Mitsyanko 
> Reported-by: Intel Security ATR 
> Reviewed-by: Stefan Hajnoczi 
> ---
> 
>  hw/sd/sdhci.c | 10 ++
>  1 file changed, 10 insertions(+)

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

Re: [Qemu-devel] [PATCH v1 1/1] sdhci.c: Limit the maximum block size

[Stefan Hajnoczi]

On Tue, Oct 06, 2015 at 11:34:46AM -0700, Peter Crosthwaite wrote:
> On Tue, Oct 6, 2015 at 10:40 AM, Alistair Francis
>  wrote:
> > It is possible for the guest to set an invalid block
> > size which is larger then the fifo_buffer[] array. This
> > could cause a buffer overflow.
> >
> > To avoid this limit the maximum size of the blksize variable.
> >
> > Signed-off-by: Alistair Francis 
> > Suggested-by: Igor Mitsyanko 
> > Reported-by: Intel Security ATR 
> > Reviewed-by: Stefan Hajnoczi 
> 
> Reviewed-by: Peter Crosthwaite 
> 
> With Pavan's patches and now this, the SD patches are starting to pile
> up on list. What queue do they target? target-arm (as lead/major user)
> or something block-related?

I can pick them up for now in my block pull requests.  Note that I'm not
an SD expert so I can't review/maintain the code.

Stefan

Re: [Qemu-devel] [PATCH v1 1/1] sdhci.c: Limit the maximum block size

[Alistair Francis]

On Thu, Oct 8, 2015 at 2:49 AM, Stefan Hajnoczi  wrote:
> On Tue, Oct 06, 2015 at 11:34:46AM -0700, Peter Crosthwaite wrote:
>> On Tue, Oct 6, 2015 at 10:40 AM, Alistair Francis
>>  wrote:
>> > It is possible for the guest to set an invalid block
>> > size which is larger then the fifo_buffer[] array. This
>> > could cause a buffer overflow.
>> >
>> > To avoid this limit the maximum size of the blksize variable.
>> >
>> > Signed-off-by: Alistair Francis 
>> > Suggested-by: Igor Mitsyanko 
>> > Reported-by: Intel Security ATR 
>> > Reviewed-by: Stefan Hajnoczi 
>>
>> Reviewed-by: Peter Crosthwaite 
>>
>> With Pavan's patches and now this, the SD patches are starting to pile
>> up on list. What queue do they target? target-arm (as lead/major user)
>> or something block-related?
>
> I can pick them up for now in my block pull requests.  Note that I'm not
> an SD expert so I can't review/maintain the code.

Thanks :) Applying them should be enough

Alistair

>
> Stefan
>

Re: [Qemu-devel] [PATCH v1 1/1] sdhci.c: Limit the maximum block size

[Peter Crosthwaite]

On Tue, Oct 6, 2015 at 10:40 AM, Alistair Francis
 wrote:
> It is possible for the guest to set an invalid block
> size which is larger then the fifo_buffer[] array. This
> could cause a buffer overflow.
>
> To avoid this limit the maximum size of the blksize variable.
>
> Signed-off-by: Alistair Francis 
> Suggested-by: Igor Mitsyanko 
> Reported-by: Intel Security ATR 
> Reviewed-by: Stefan Hajnoczi 

Reviewed-by: Peter Crosthwaite 

With Pavan's patches and now this, the SD patches are starting to pile
up on list. What queue do they target? target-arm (as lead/major user)
or something block-related?

Regards,
Peter

> ---
>
>  hw/sd/sdhci.c | 10 ++
>  1 file changed, 10 insertions(+)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 65304cf..1d47f5c 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1006,6 +1006,16 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, 
> unsigned size)
>  MASKED_WRITE(s->blksize, mask, value);
>  MASKED_WRITE(s->blkcnt, mask >> 16, value >> 16);
>  }
> +
> +/* Limit block size to the maximum buffer size */
> +if (extract32(s->blksize, 0, 12) > s->buf_maxsz) {
> +qemu_log_mask(LOG_GUEST_ERROR, "%s: Size 0x%x is larger than " \
> +  "the maximum buffer 0x%x", __func__, s->blksize,
> +  s->buf_maxsz);
> +
> +s->blksize = deposit32(s->blksize, 0, 12, s->buf_maxsz);
> +}
> +
>  break;
>  case SDHC_ARGUMENT:
>  MASKED_WRITE(s->argument, mask, value);
> --
> 2.1.4
>

[Qemu-devel] [PATCH v1 1/1] sdhci.c: Limit the maximum block size

[Alistair Francis]

It is possible for the guest to set an invalid block
size which is larger then the fifo_buffer[] array. This
could cause a buffer overflow.

To avoid this limit the maximum size of the blksize variable.

Signed-off-by: Alistair Francis 
Suggested-by: Igor Mitsyanko 
Reported-by: Intel Security ATR 
Reviewed-by: Stefan Hajnoczi 
---

 hw/sd/sdhci.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 65304cf..1d47f5c 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1006,6 +1006,16 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, 
unsigned size)
 MASKED_WRITE(s->blksize, mask, value);
 MASKED_WRITE(s->blkcnt, mask >> 16, value >> 16);
 }
+
+/* Limit block size to the maximum buffer size */
+if (extract32(s->blksize, 0, 12) > s->buf_maxsz) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: Size 0x%x is larger than " \
+  "the maximum buffer 0x%x", __func__, s->blksize,
+  s->buf_maxsz);
+
+s->blksize = deposit32(s->blksize, 0, 12, s->buf_maxsz);
+}
+
 break;
 case SDHC_ARGUMENT:
 MASKED_WRITE(s->argument, mask, value);
-- 
2.1.4