1. Patchew
  2. QEMU
  3. View series

[PATCH] hw/sd/sdhci: reset data count in sdhci_buff_access_is_sequential()

Mauro Matteo Cascella posted 1 patch 1 year, 5 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20221107103510.34588-1-mcascell@redhat.com
Maintainers: "Philippe Mathieu-Daudé" <philmd@linaro.org>, Bin Meng <bin.meng@windriver.com>
hw/sd/sdhci.c | 4 ++++
1 file changed, 4 insertions(+)
[PATCH] hw/sd/sdhci: reset data count in sdhci_buff_access_is_sequential()
Posted by Mauro Matteo Cascella 1 year, 5 months ago 
Make sure to reset data_count if it's equal to (or exceeds) block_size.
This prevents an off-by-one read / write when accessing s->fifo_buffer
in sdhci_read_dataport / sdhci_write_dataport, both called right after
sdhci_buff_access_is_sequential.

Fixes: CVE-2022-3872
Reported-by: RivenDell <XRivenDell@outlook.com>
Reported-by: Siqi Chen <coc.cyqh@gmail.com>
Reported-by: ningqiang <ningqiang1@huawei.com>
Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
---
 hw/sd/sdhci.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 306070c872..aa2fd79df2 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -978,6 +978,10 @@ static bool sdhci_can_issue_command(SDHCIState *s)
 static inline bool
 sdhci_buff_access_is_sequential(SDHCIState *s, unsigned byte_num)
 {
+    if (s->data_count >= (s->blksize & BLOCK_SIZE_MASK)) {
+        s->data_count = 0;
+    }
+
     if ((s->data_count & 0x3) != byte_num) {
         trace_sdhci_error("Non-sequential access to Buffer Data Port register"
                           "is prohibited\n");
-- 
2.38.1
Re: [PATCH] hw/sd/sdhci: reset data count in sdhci_buff_access_is_sequential()
Posted by Philippe Mathieu-Daudé 1 year, 5 months ago 
On 7/11/22 11:35, Mauro Matteo Cascella wrote:
> Make sure to reset data_count if it's equal to (or exceeds) block_size.
> This prevents an off-by-one read / write when accessing s->fifo_buffer
> in sdhci_read_dataport / sdhci_write_dataport, both called right after
> sdhci_buff_access_is_sequential.
> 
> Fixes: CVE-2022-3872
> Reported-by: RivenDell <XRivenDell@outlook.com>
> Reported-by: Siqi Chen <coc.cyqh@gmail.com>
> Reported-by: ningqiang <ningqiang1@huawei.com>
> Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
> ---
>   hw/sd/sdhci.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 306070c872..aa2fd79df2 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -978,6 +978,10 @@ static bool sdhci_can_issue_command(SDHCIState *s)
>   static inline bool
>   sdhci_buff_access_is_sequential(SDHCIState *s, unsigned byte_num)
>   {
> +    if (s->data_count >= (s->blksize & BLOCK_SIZE_MASK)) {
> +        s->data_count = 0;
> +    }

You avoid an off-by-one but now the model doesn't work per the spec.
Not really what the best fix IMHO.

>       if ((s->data_count & 0x3) != byte_num) {
>           trace_sdhci_error("Non-sequential access to Buffer Data Port register"
>                             "is prohibited\n");

I wonder why sdhci_data_transfer() indiscriminately sets
SDHC_SPACE_AVAILABLE in the write path (at least without
clearing the FIFO first).

The fix could be:

-- >8 --
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
@@ -955,5 +955,5 @@ static void sdhci_data_transfer(void *opaque)
          } else {
              s->prnsts |= SDHC_DOING_WRITE | SDHC_DAT_LINE_ACTIVE |
-                    SDHC_SPACE_AVAILABLE | SDHC_DATA_INHIBIT;
+                                           SDHC_DATA_INHIBIT;
              sdhci_write_block_to_card(s);
          }
---

Bin, what do you think?

Regards,

Phil.
Re: [PATCH] hw/sd/sdhci: reset data count in sdhci_buff_access_is_sequential()
Posted by Mauro Matteo Cascella 1 year, 5 months ago 
On Mon, Nov 7, 2022 at 8:12 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 7/11/22 11:35, Mauro Matteo Cascella wrote:
> > Make sure to reset data_count if it's equal to (or exceeds) block_size.
> > This prevents an off-by-one read / write when accessing s->fifo_buffer
> > in sdhci_read_dataport / sdhci_write_dataport, both called right after
> > sdhci_buff_access_is_sequential.
> >
> > Fixes: CVE-2022-3872
> > Reported-by: RivenDell <XRivenDell@outlook.com>
> > Reported-by: Siqi Chen <coc.cyqh@gmail.com>
> > Reported-by: ningqiang <ningqiang1@huawei.com>
> > Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
> > ---
> >   hw/sd/sdhci.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> >
> > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> > index 306070c872..aa2fd79df2 100644
> > --- a/hw/sd/sdhci.c
> > +++ b/hw/sd/sdhci.c
> > @@ -978,6 +978,10 @@ static bool sdhci_can_issue_command(SDHCIState *s)
> >   static inline bool
> >   sdhci_buff_access_is_sequential(SDHCIState *s, unsigned byte_num)
> >   {
> > +    if (s->data_count >= (s->blksize & BLOCK_SIZE_MASK)) {
> > +        s->data_count = 0;
> > +    }
>
> You avoid an off-by-one but now the model doesn't work per the spec.

Note that a similar thing is done in sdhci_{read,write}_dataport. But
it's true that this is probably not enough (e.g. no update of
s->prnsts). Thank you for sending
https://lists.nongnu.org/archive/html/qemu-devel/2022-11/msg01161.html.

> Not really what the best fix IMHO.
>
> >       if ((s->data_count & 0x3) != byte_num) {
> >           trace_sdhci_error("Non-sequential access to Buffer Data Port register"
> >                             "is prohibited\n");
>
> I wonder why sdhci_data_transfer() indiscriminately sets
> SDHC_SPACE_AVAILABLE in the write path (at least without
> clearing the FIFO first).
>
> The fix could be:
>
> -- >8 --
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> @@ -955,5 +955,5 @@ static void sdhci_data_transfer(void *opaque)
>           } else {
>               s->prnsts |= SDHC_DOING_WRITE | SDHC_DAT_LINE_ACTIVE |
> -                    SDHC_SPACE_AVAILABLE | SDHC_DATA_INHIBIT;
> +                                           SDHC_DATA_INHIBIT;
>               sdhci_write_block_to_card(s);
>           }
> ---
>
> Bin, what do you think?
>
> Regards,
>
> Phil.
>

--
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0
Re: [PATCH] hw/sd/sdhci: reset data count in sdhci_buff_access_is_sequential()
Posted by Mauro Matteo Cascella 1 year, 5 months ago 
On Mon, Nov 7, 2022 at 11:35 AM Mauro Matteo Cascella
<mcascell@redhat.com> wrote:
>
> Make sure to reset data_count if it's equal to (or exceeds) block_size.
> This prevents an off-by-one read / write when accessing s->fifo_buffer
> in sdhci_read_dataport / sdhci_write_dataport, both called right after
> sdhci_buff_access_is_sequential.
>
> Fixes: CVE-2022-3872
> Reported-by: RivenDell <XRivenDell@outlook.com>
> Reported-by: Siqi Chen <coc.cyqh@gmail.com>
> Reported-by: ningqiang <ningqiang1@huawei.com>
> Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
> ---
>  hw/sd/sdhci.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 306070c872..aa2fd79df2 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -978,6 +978,10 @@ static bool sdhci_can_issue_command(SDHCIState *s)
>  static inline bool
>  sdhci_buff_access_is_sequential(SDHCIState *s, unsigned byte_num)
>  {
> +    if (s->data_count >= (s->blksize & BLOCK_SIZE_MASK)) {
> +        s->data_count = 0;
> +    }
> +
>      if ((s->data_count & 0x3) != byte_num) {
>          trace_sdhci_error("Non-sequential access to Buffer Data Port register"
>                            "is prohibited\n");
> --
> 2.38.1
>

Reproducer:

cat << EOF | ./qemu-system-x86_64 -machine accel=qtest \
-nodefaults -drive if=none,index=0,file=null-co://,format=raw,id=mydrive \
-device sdhci-pci -device sd-card,drive=mydrive -nographic -qtest stdio
outl 0xcf8 0x80001004
outl 0xcfc 0x107
outl 0xcf8 0x80001010
outl 0xcfc 0xfebf1000
writel 0xfebf102c 0x7
writel 0xfebf1004 0x10200
writel 0xfebf100c 0x200000
writel 0xfebf1028 0x10000
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1020 0xdeadbeef
writel 0xfebf1004 0x200
writel 0xfebf100c 0x20
writel 0xfebf1028 0x20000
writel 0x00100000 0xfebf1021
writel 0xfebf1058 0x00100000
writel 0xfebf1028 0x8
writel 0xfebf100c 0x200011
writel 0xfebf1020 0xaabbccdd
EOF

-- 
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0
Re: [PATCH] hw/sd/sdhci: reset data count in sdhci_buff_access_is_sequential()
Posted by Bin Meng 1 year, 5 months ago 
Hi,

On Mon, Nov 7, 2022 at 7:08 PM Mauro Matteo Cascella
<mcascell@redhat.com> wrote:
>
> On Mon, Nov 7, 2022 at 11:35 AM Mauro Matteo Cascella
> <mcascell@redhat.com> wrote:
> >
> > Make sure to reset data_count if it's equal to (or exceeds) block_size.
> > This prevents an off-by-one read / write when accessing s->fifo_buffer
> > in sdhci_read_dataport / sdhci_write_dataport, both called right after
> > sdhci_buff_access_is_sequential.
> >
> > Fixes: CVE-2022-3872
> > Reported-by: RivenDell <XRivenDell@outlook.com>
> > Reported-by: Siqi Chen <coc.cyqh@gmail.com>
> > Reported-by: ningqiang <ningqiang1@huawei.com>
> > Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
> > ---
> >  hw/sd/sdhci.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> > index 306070c872..aa2fd79df2 100644
> > --- a/hw/sd/sdhci.c
> > +++ b/hw/sd/sdhci.c
> > @@ -978,6 +978,10 @@ static bool sdhci_can_issue_command(SDHCIState *s)
> >  static inline bool
> >  sdhci_buff_access_is_sequential(SDHCIState *s, unsigned byte_num)
> >  {
> > +    if (s->data_count >= (s->blksize & BLOCK_SIZE_MASK)) {
> > +        s->data_count = 0;
> > +    }
> > +
> >      if ((s->data_count & 0x3) != byte_num) {
> >          trace_sdhci_error("Non-sequential access to Buffer Data Port register"
> >                            "is prohibited\n");
> > --
> > 2.38.1
> >
>
> Reproducer:
>
> cat << EOF | ./qemu-system-x86_64 -machine accel=qtest \
> -nodefaults -drive if=none,index=0,file=null-co://,format=raw,id=mydrive \
> -device sdhci-pci -device sd-card,drive=mydrive -nographic -qtest stdio
> outl 0xcf8 0x80001004
> outl 0xcfc 0x107
> outl 0xcf8 0x80001010
> outl 0xcfc 0xfebf1000
> writel 0xfebf102c 0x7
> writel 0xfebf1004 0x10200
> writel 0xfebf100c 0x200000
> writel 0xfebf1028 0x10000
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1020 0xdeadbeef
> writel 0xfebf1004 0x200
> writel 0xfebf100c 0x20
> writel 0xfebf1028 0x20000
> writel 0x00100000 0xfebf1021
> writel 0xfebf1058 0x00100000
> writel 0xfebf1028 0x8
> writel 0xfebf100c 0x200011
> writel 0xfebf1020 0xaabbccdd
> EOF
>

This reproducer does not crash my QEMU. Am I missing anything?

Regards,
Bin
Re: [PATCH] hw/sd/sdhci: reset data count in sdhci_buff_access_is_sequential()
Posted by Siqi Chen 1 year, 5 months ago 
Hi,

>This reproducer does not crash my QEMU. Am I missing anything?
I submitted the reproducer. Because the overflow is only one byte, it may
not be detected by the host's heap allocator.  Do you compile your qemu
with sanitizer?  This is my build configuration: "./configure
--target-list=x86_64-softmmu --enable-sanitizers"

Thanks,
Siqi Chen.



Bin Meng <bmeng.cn@gmail.com> 于2022年11月9日周三 17:30写道：

> Hi,
>
> On Mon, Nov 7, 2022 at 7:08 PM Mauro Matteo Cascella
> <mcascell@redhat.com> wrote:
> >
> > On Mon, Nov 7, 2022 at 11:35 AM Mauro Matteo Cascella
> > <mcascell@redhat.com> wrote:
> > >
> > > Make sure to reset data_count if it's equal to (or exceeds) block_size.
> > > This prevents an off-by-one read / write when accessing s->fifo_buffer
> > > in sdhci_read_dataport / sdhci_write_dataport, both called right after
> > > sdhci_buff_access_is_sequential.
> > >
> > > Fixes: CVE-2022-3872
> > > Reported-by: RivenDell <XRivenDell@outlook.com>
> > > Reported-by: Siqi Chen <coc.cyqh@gmail.com>
> > > Reported-by: ningqiang <ningqiang1@huawei.com>
> > > Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
> > > ---
> > >  hw/sd/sdhci.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> > > index 306070c872..aa2fd79df2 100644
> > > --- a/hw/sd/sdhci.c
> > > +++ b/hw/sd/sdhci.c
> > > @@ -978,6 +978,10 @@ static bool sdhci_can_issue_command(SDHCIState *s)
> > >  static inline bool
> > >  sdhci_buff_access_is_sequential(SDHCIState *s, unsigned byte_num)
> > >  {
> > > +    if (s->data_count >= (s->blksize & BLOCK_SIZE_MASK)) {
> > > +        s->data_count = 0;
> > > +    }
> > > +
> > >      if ((s->data_count & 0x3) != byte_num) {
> > >          trace_sdhci_error("Non-sequential access to Buffer Data Port
> register"
> > >                            "is prohibited\n");
> > > --
> > > 2.38.1
> > >
> >
> > Reproducer:
> >
> > cat << EOF | ./qemu-system-x86_64 -machine accel=qtest \
> > -nodefaults -drive if=none,index=0,file=null-co://,format=raw,id=mydrive
> \
> > -device sdhci-pci -device sd-card,drive=mydrive -nographic -qtest stdio
> > outl 0xcf8 0x80001004
> > outl 0xcfc 0x107
> > outl 0xcf8 0x80001010
> > outl 0xcfc 0xfebf1000
> > writel 0xfebf102c 0x7
> > writel 0xfebf1004 0x10200
> > writel 0xfebf100c 0x200000
> > writel 0xfebf1028 0x10000
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1020 0xdeadbeef
> > writel 0xfebf1004 0x200
> > writel 0xfebf100c 0x20
> > writel 0xfebf1028 0x20000
> > writel 0x00100000 0xfebf1021
> > writel 0xfebf1058 0x00100000
> > writel 0xfebf1028 0x8
> > writel 0xfebf100c 0x200011
> > writel 0xfebf1020 0xaabbccdd
> > EOF
> >
>
> This reproducer does not crash my QEMU. Am I missing anything?
>
> Regards,
> Bin
>
Re: [PATCH] hw/sd/sdhci: reset data count in sdhci_buff_access_is_sequential()
Posted by Mauro Matteo Cascella 1 year, 5 months ago 
On Wed, Nov 9, 2022 at 10:45 AM Siqi Chen <coc.cyqh@gmail.com> wrote:
>
> Hi,
>
> >This reproducer does not crash my QEMU. Am I missing anything?
> I submitted the reproducer. Because the overflow is only one byte, it may not be detected by the host's heap allocator.  Do you compile your qemu with sanitizer?  This is my build configuration: "./configure --target-list=x86_64-softmmu --enable-sanitizers"

Right, you need to recompile QEMU with ASAN support. This is an
excerpt of the stack trace:

=================================================================
==39159==ERROR: AddressSanitizer: heap-buffer-overflow on address
0x615000022880 at pc 0x55b023db5fe1 bp 0x7fffeeef1650 sp
0x7fffeeef1648
WRITE of size 1 at 0x615000022880 thread T0
    #0 0x55b023db5fe0 in sdhci_write_dataport ../../hw/sd/sdhci.c:562
    #1 0x55b023dc1cc7 in sdhci_write ../../hw/sd/sdhci.c:1216
    #2 0x55b024521e01 in memory_region_write_accessor ../../softmmu/memory.c:492
    #3 0x55b0245222ab in access_with_adjusted_size ../../softmmu/memory.c:554
    #4 0x55b02452ff15 in memory_region_dispatch_write
../../softmmu/memory.c:1514
    #5 0x55b024568c67 in flatview_write_continue ../../softmmu/physmem.c:2814
    #6 0x55b02456908d in flatview_write ../../softmmu/physmem.c:2856
    #7 0x55b024569a74 in address_space_write ../../softmmu/physmem.c:2952
    #8 0x55b02457a63c in qtest_process_command ../../softmmu/qtest.c:538
    #9 0x55b02457ef97 in qtest_process_inbuf ../../softmmu/qtest.c:796
    #10 0x55b02457f089 in qtest_read ../../softmmu/qtest.c:808
    #11 0x55b0249d4372 in qemu_chr_be_write_impl ../../chardev/char.c:201
    #12 0x55b0249d4414 in qemu_chr_be_write ../../chardev/char.c:213
    #13 0x55b0249db586 in fd_chr_read ../../chardev/char-fd.c:72
    #14 0x55b02466ba5b in qio_channel_fd_source_dispatch
../../io/channel-watch.c:84
    #15 0x7f88093af0ae in g_main_context_dispatch
(/lib64/libglib-2.0.so.0+0x550ae)
    #16 0x55b024c5ff14 in glib_pollfds_poll ../../util/main-loop.c:297
    #17 0x55b024c600fa in os_host_main_loop_wait ../../util/main-loop.c:320
    #18 0x55b024c603f3 in main_loop_wait ../../util/main-loop.c:596
    #19 0x55b023fcca21 in qemu_main_loop ../../softmmu/runstate.c:726
    #20 0x55b023679735 in qemu_main ../../softmmu/main.c:36
    #21 0x55b023679766 in main ../../softmmu/main.c:45
    #22 0x7f8808728f5f in __libc_start_call_main (/lib64/libc.so.6+0x40f5f)
    #23 0x7f880872900f in __libc_start_main_impl (/lib64/libc.so.6+0x4100f)
    #24 0x55b023679644 in _start (./qemu-system-x86_64+0x20f2644)

> Thanks,
> Siqi Chen.
>
>
>
> Bin Meng <bmeng.cn@gmail.com> 于2022年11月9日周三 17:30写道：
>>
>> Hi,
>>
>> On Mon, Nov 7, 2022 at 7:08 PM Mauro Matteo Cascella
>> <mcascell@redhat.com> wrote:
>> >
>> > On Mon, Nov 7, 2022 at 11:35 AM Mauro Matteo Cascella
>> > <mcascell@redhat.com> wrote:
>> > >
>> > > Make sure to reset data_count if it's equal to (or exceeds) block_size.
>> > > This prevents an off-by-one read / write when accessing s->fifo_buffer
>> > > in sdhci_read_dataport / sdhci_write_dataport, both called right after
>> > > sdhci_buff_access_is_sequential.
>> > >
>> > > Fixes: CVE-2022-3872
>> > > Reported-by: RivenDell <XRivenDell@outlook.com>
>> > > Reported-by: Siqi Chen <coc.cyqh@gmail.com>
>> > > Reported-by: ningqiang <ningqiang1@huawei.com>
>> > > Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
>> > > ---
>> > >  hw/sd/sdhci.c | 4 ++++
>> > >  1 file changed, 4 insertions(+)
>> > >
>> > > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>> > > index 306070c872..aa2fd79df2 100644
>> > > --- a/hw/sd/sdhci.c
>> > > +++ b/hw/sd/sdhci.c
>> > > @@ -978,6 +978,10 @@ static bool sdhci_can_issue_command(SDHCIState *s)
>> > >  static inline bool
>> > >  sdhci_buff_access_is_sequential(SDHCIState *s, unsigned byte_num)
>> > >  {
>> > > +    if (s->data_count >= (s->blksize & BLOCK_SIZE_MASK)) {
>> > > +        s->data_count = 0;
>> > > +    }
>> > > +
>> > >      if ((s->data_count & 0x3) != byte_num) {
>> > >          trace_sdhci_error("Non-sequential access to Buffer Data Port register"
>> > >                            "is prohibited\n");
>> > > --
>> > > 2.38.1
>> > >
>> >
>> > Reproducer:
>> >
>> > cat << EOF | ./qemu-system-x86_64 -machine accel=qtest \
>> > -nodefaults -drive if=none,index=0,file=null-co://,format=raw,id=mydrive \
>> > -device sdhci-pci -device sd-card,drive=mydrive -nographic -qtest stdio
>> > outl 0xcf8 0x80001004
>> > outl 0xcfc 0x107
>> > outl 0xcf8 0x80001010
>> > outl 0xcfc 0xfebf1000
>> > writel 0xfebf102c 0x7
>> > writel 0xfebf1004 0x10200
>> > writel 0xfebf100c 0x200000
>> > writel 0xfebf1028 0x10000
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1020 0xdeadbeef
>> > writel 0xfebf1004 0x200
>> > writel 0xfebf100c 0x20
>> > writel 0xfebf1028 0x20000
>> > writel 0x00100000 0xfebf1021
>> > writel 0xfebf1058 0x00100000
>> > writel 0xfebf1028 0x8
>> > writel 0xfebf100c 0x200011
>> > writel 0xfebf1020 0xaabbccdd
>> > EOF
>> >
>>
>> This reproducer does not crash my QEMU. Am I missing anything?
>>
>> Regards,
>> Bin



-- 
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0
Re: [PATCH] hw/sd/sdhci: reset data count in sdhci_buff_access_is_sequential()
Posted by Bin Meng 1 year, 5 months ago 
On Wed, Nov 9, 2022 at 6:10 PM Mauro Matteo Cascella
<mcascell@redhat.com> wrote:
>
> On Wed, Nov 9, 2022 at 10:45 AM Siqi Chen <coc.cyqh@gmail.com> wrote:
> >
> > Hi,
> >
> > >This reproducer does not crash my QEMU. Am I missing anything?
> > I submitted the reproducer. Because the overflow is only one byte, it may not be detected by the host's heap allocator.  Do you compile your qemu with sanitizer?  This is my build configuration: "./configure --target-list=x86_64-softmmu --enable-sanitizers"
>
> Right, you need to recompile QEMU with ASAN support. This is an
> excerpt of the stack trace:

Is this documented somewhere? Is fuzzing.rst the right doc for this
feature? Looking at fuzzing.rst it says --enable-sanitizers is
optional.

Turning on --enable-sanitizers makes the build fail:

FAILED: subprojects/libvduse/libvduse.a.p/libvduse.c.o
cc -m64 -mcx16 -Isubprojects/libvduse/libvduse.a.p
-Isubprojects/libvduse -I../subprojects/libvduse
-fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g
-fsanitize=undefined -fsanitize=address -U_FORTIFY
_SOURCE -D_FORTIFY_SOURCE=2 -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE
-Wstrict-prototypes -Wredundant-decls -Wundef -Wwrite-strings
-Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv
-Wold-style-declaration -W
old-style-definition -Wtype-limits -Wformat-security -Wformat-y2k
-Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs
-Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2
-Wno-missing-include-dirs -Wn
o-shift-negative-value -Wno-psabi -fstack-protector-strong -fPIE
-D_GNU_SOURCE -MD -MQ subprojects/libvduse/libvduse.a.p/libvduse.c.o
-MF subprojects/libvduse/libvduse.a.p/libvduse.c.o.d -o
subprojects/libvduse/libvduse.a
.p/libvduse.c.o -c ../subprojects/libvduse/libvduse.c
In file included from /usr/include/string.h:495,
from ../subprojects/libvduse/libvduse.c:24:
In function ‘strncpy’,
inlined from ‘vduse_dev_create’ at ../subprojects/libvduse/libvduse.c:1312:5:
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:106:10: error:
‘__builtin_strncpy’ specified bound 256 equals destination size
[-Werror=stringop-truncation]
106 | return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

I am using GCC 9.4 on Ubuntu 20.04

>
> =================================================================
> ==39159==ERROR: AddressSanitizer: heap-buffer-overflow on address
> 0x615000022880 at pc 0x55b023db5fe1 bp 0x7fffeeef1650 sp
> 0x7fffeeef1648
> WRITE of size 1 at 0x615000022880 thread T0
>     #0 0x55b023db5fe0 in sdhci_write_dataport ../../hw/sd/sdhci.c:562
>     #1 0x55b023dc1cc7 in sdhci_write ../../hw/sd/sdhci.c:1216
>     #2 0x55b024521e01 in memory_region_write_accessor ../../softmmu/memory.c:492
>     #3 0x55b0245222ab in access_with_adjusted_size ../../softmmu/memory.c:554
>     #4 0x55b02452ff15 in memory_region_dispatch_write
> ../../softmmu/memory.c:1514
>     #5 0x55b024568c67 in flatview_write_continue ../../softmmu/physmem.c:2814
>     #6 0x55b02456908d in flatview_write ../../softmmu/physmem.c:2856
>     #7 0x55b024569a74 in address_space_write ../../softmmu/physmem.c:2952
>     #8 0x55b02457a63c in qtest_process_command ../../softmmu/qtest.c:538
>     #9 0x55b02457ef97 in qtest_process_inbuf ../../softmmu/qtest.c:796
>     #10 0x55b02457f089 in qtest_read ../../softmmu/qtest.c:808
>     #11 0x55b0249d4372 in qemu_chr_be_write_impl ../../chardev/char.c:201
>     #12 0x55b0249d4414 in qemu_chr_be_write ../../chardev/char.c:213
>     #13 0x55b0249db586 in fd_chr_read ../../chardev/char-fd.c:72
>     #14 0x55b02466ba5b in qio_channel_fd_source_dispatch
> ../../io/channel-watch.c:84
>     #15 0x7f88093af0ae in g_main_context_dispatch
> (/lib64/libglib-2.0.so.0+0x550ae)
>     #16 0x55b024c5ff14 in glib_pollfds_poll ../../util/main-loop.c:297
>     #17 0x55b024c600fa in os_host_main_loop_wait ../../util/main-loop.c:320
>     #18 0x55b024c603f3 in main_loop_wait ../../util/main-loop.c:596
>     #19 0x55b023fcca21 in qemu_main_loop ../../softmmu/runstate.c:726
>     #20 0x55b023679735 in qemu_main ../../softmmu/main.c:36
>     #21 0x55b023679766 in main ../../softmmu/main.c:45
>     #22 0x7f8808728f5f in __libc_start_call_main (/lib64/libc.so.6+0x40f5f)
>     #23 0x7f880872900f in __libc_start_main_impl (/lib64/libc.so.6+0x4100f)
>     #24 0x55b023679644 in _start (./qemu-system-x86_64+0x20f2644)
>

Regards,
Bin
Re: [PATCH] hw/sd/sdhci: reset data count in sdhci_buff_access_is_sequential()
Posted by Mauro Matteo Cascella 1 year, 5 months ago 
On Wed, Nov 9, 2022 at 5:19 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Wed, Nov 9, 2022 at 6:10 PM Mauro Matteo Cascella
> <mcascell@redhat.com> wrote:
> >
> > On Wed, Nov 9, 2022 at 10:45 AM Siqi Chen <coc.cyqh@gmail.com> wrote:
> > >
> > > Hi,
> > >
> > > >This reproducer does not crash my QEMU. Am I missing anything?
> > > I submitted the reproducer. Because the overflow is only one byte, it may not be detected by the host's heap allocator.  Do you compile your qemu with sanitizer?  This is my build configuration: "./configure --target-list=x86_64-softmmu --enable-sanitizers"
> >
> > Right, you need to recompile QEMU with ASAN support. This is an
> > excerpt of the stack trace:
>
> Is this documented somewhere? Is fuzzing.rst the right doc for this
> feature? Looking at fuzzing.rst it says --enable-sanitizers is
> optional.

Not sure it's documented somewhere, this is how I usually compile:

$ ../configure --target-list=x86_64-softmmu --enable-sanitizers
--extra-cflags=-g3 \
 --enable-kvm --disable-tcg --enable-debug --enable-debug-info --disable-werror

Then just run the PoC using ./x86_64-softmmu/qemu-system-x86_64 should
do the trick.

> Turning on --enable-sanitizers makes the build fail:
>
> FAILED: subprojects/libvduse/libvduse.a.p/libvduse.c.o
> cc -m64 -mcx16 -Isubprojects/libvduse/libvduse.a.p
> -Isubprojects/libvduse -I../subprojects/libvduse
> -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g
> -fsanitize=undefined -fsanitize=address -U_FORTIFY
> _SOURCE -D_FORTIFY_SOURCE=2 -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE
> -Wstrict-prototypes -Wredundant-decls -Wundef -Wwrite-strings
> -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv
> -Wold-style-declaration -W
> old-style-definition -Wtype-limits -Wformat-security -Wformat-y2k
> -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs
> -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2
> -Wno-missing-include-dirs -Wn
> o-shift-negative-value -Wno-psabi -fstack-protector-strong -fPIE
> -D_GNU_SOURCE -MD -MQ subprojects/libvduse/libvduse.a.p/libvduse.c.o
> -MF subprojects/libvduse/libvduse.a.p/libvduse.c.o.d -o
> subprojects/libvduse/libvduse.a
> .p/libvduse.c.o -c ../subprojects/libvduse/libvduse.c
> In file included from /usr/include/string.h:495,
> from ../subprojects/libvduse/libvduse.c:24:
> In function ‘strncpy’,
> inlined from ‘vduse_dev_create’ at ../subprojects/libvduse/libvduse.c:1312:5:
> /usr/include/x86_64-linux-gnu/bits/string_fortified.h:106:10: error:
> ‘__builtin_strncpy’ specified bound 256 equals destination size
> [-Werror=stringop-truncation]
> 106 | return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
>
> I am using GCC 9.4 on Ubuntu 20.04
>
> >
> > =================================================================
> > ==39159==ERROR: AddressSanitizer: heap-buffer-overflow on address
> > 0x615000022880 at pc 0x55b023db5fe1 bp 0x7fffeeef1650 sp
> > 0x7fffeeef1648
> > WRITE of size 1 at 0x615000022880 thread T0
> >     #0 0x55b023db5fe0 in sdhci_write_dataport ../../hw/sd/sdhci.c:562
> >     #1 0x55b023dc1cc7 in sdhci_write ../../hw/sd/sdhci.c:1216
> >     #2 0x55b024521e01 in memory_region_write_accessor ../../softmmu/memory.c:492
> >     #3 0x55b0245222ab in access_with_adjusted_size ../../softmmu/memory.c:554
> >     #4 0x55b02452ff15 in memory_region_dispatch_write
> > ../../softmmu/memory.c:1514
> >     #5 0x55b024568c67 in flatview_write_continue ../../softmmu/physmem.c:2814
> >     #6 0x55b02456908d in flatview_write ../../softmmu/physmem.c:2856
> >     #7 0x55b024569a74 in address_space_write ../../softmmu/physmem.c:2952
> >     #8 0x55b02457a63c in qtest_process_command ../../softmmu/qtest.c:538
> >     #9 0x55b02457ef97 in qtest_process_inbuf ../../softmmu/qtest.c:796
> >     #10 0x55b02457f089 in qtest_read ../../softmmu/qtest.c:808
> >     #11 0x55b0249d4372 in qemu_chr_be_write_impl ../../chardev/char.c:201
> >     #12 0x55b0249d4414 in qemu_chr_be_write ../../chardev/char.c:213
> >     #13 0x55b0249db586 in fd_chr_read ../../chardev/char-fd.c:72
> >     #14 0x55b02466ba5b in qio_channel_fd_source_dispatch
> > ../../io/channel-watch.c:84
> >     #15 0x7f88093af0ae in g_main_context_dispatch
> > (/lib64/libglib-2.0.so.0+0x550ae)
> >     #16 0x55b024c5ff14 in glib_pollfds_poll ../../util/main-loop.c:297
> >     #17 0x55b024c600fa in os_host_main_loop_wait ../../util/main-loop.c:320
> >     #18 0x55b024c603f3 in main_loop_wait ../../util/main-loop.c:596
> >     #19 0x55b023fcca21 in qemu_main_loop ../../softmmu/runstate.c:726
> >     #20 0x55b023679735 in qemu_main ../../softmmu/main.c:36
> >     #21 0x55b023679766 in main ../../softmmu/main.c:45
> >     #22 0x7f8808728f5f in __libc_start_call_main (/lib64/libc.so.6+0x40f5f)
> >     #23 0x7f880872900f in __libc_start_main_impl (/lib64/libc.so.6+0x4100f)
> >     #24 0x55b023679644 in _start (./qemu-system-x86_64+0x20f2644)
> >
>
> Regards,
> Bin
>


-- 
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0
Re: [PATCH] hw/sd/sdhci: reset data count in sdhci_buff_access_is_sequential()
Posted by Bin Meng 1 year, 5 months ago 
On Fri, Nov 11, 2022 at 2:51 AM Mauro Matteo Cascella
<mcascell@redhat.com> wrote:
>
> On Wed, Nov 9, 2022 at 5:19 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > On Wed, Nov 9, 2022 at 6:10 PM Mauro Matteo Cascella
> > <mcascell@redhat.com> wrote:
> > >
> > > On Wed, Nov 9, 2022 at 10:45 AM Siqi Chen <coc.cyqh@gmail.com> wrote:
> > > >
> > > > Hi,
> > > >
> > > > >This reproducer does not crash my QEMU. Am I missing anything?
> > > > I submitted the reproducer. Because the overflow is only one byte, it may not be detected by the host's heap allocator.  Do you compile your qemu with sanitizer?  This is my build configuration: "./configure --target-list=x86_64-softmmu --enable-sanitizers"
> > >
> > > Right, you need to recompile QEMU with ASAN support. This is an
> > > excerpt of the stack trace:
> >
> > Is this documented somewhere? Is fuzzing.rst the right doc for this
> > feature? Looking at fuzzing.rst it says --enable-sanitizers is
> > optional.
>
> Not sure it's documented somewhere, this is how I usually compile:
>
> $ ../configure --target-list=x86_64-softmmu --enable-sanitizers
> --extra-cflags=-g3 \
>  --enable-kvm --disable-tcg --enable-debug --enable-debug-info --disable-werror

Thanks, --disable-werror makes the build pass and I can reproduce this issue.

I can prepare a patch to improve the doc with the build instructions.

>
> Then just run the PoC using ./x86_64-softmmu/qemu-system-x86_64 should
> do the trick.
>
> > Turning on --enable-sanitizers makes the build fail:
> >
> > FAILED: subprojects/libvduse/libvduse.a.p/libvduse.c.o
> > cc -m64 -mcx16 -Isubprojects/libvduse/libvduse.a.p
> > -Isubprojects/libvduse -I../subprojects/libvduse
> > -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g
> > -fsanitize=undefined -fsanitize=address -U_FORTIFY
> > _SOURCE -D_FORTIFY_SOURCE=2 -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE
> > -Wstrict-prototypes -Wredundant-decls -Wundef -Wwrite-strings
> > -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv
> > -Wold-style-declaration -W
> > old-style-definition -Wtype-limits -Wformat-security -Wformat-y2k
> > -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs
> > -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2
> > -Wno-missing-include-dirs -Wn
> > o-shift-negative-value -Wno-psabi -fstack-protector-strong -fPIE
> > -D_GNU_SOURCE -MD -MQ subprojects/libvduse/libvduse.a.p/libvduse.c.o
> > -MF subprojects/libvduse/libvduse.a.p/libvduse.c.o.d -o
> > subprojects/libvduse/libvduse.a
> > .p/libvduse.c.o -c ../subprojects/libvduse/libvduse.c
> > In file included from /usr/include/string.h:495,
> > from ../subprojects/libvduse/libvduse.c:24:
> > In function ‘strncpy’,
> > inlined from ‘vduse_dev_create’ at ../subprojects/libvduse/libvduse.c:1312:5:
> > /usr/include/x86_64-linux-gnu/bits/string_fortified.h:106:10: error:
> > ‘__builtin_strncpy’ specified bound 256 equals destination size
> > [-Werror=stringop-truncation]
> > 106 | return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > cc1: all warnings being treated as errors
> >
> > I am using GCC 9.4 on Ubuntu 20.04
> >
> > >
> > > =================================================================
> > > ==39159==ERROR: AddressSanitizer: heap-buffer-overflow on address
> > > 0x615000022880 at pc 0x55b023db5fe1 bp 0x7fffeeef1650 sp
> > > 0x7fffeeef1648
> > > WRITE of size 1 at 0x615000022880 thread T0
> > >     #0 0x55b023db5fe0 in sdhci_write_dataport ../../hw/sd/sdhci.c:562
> > >     #1 0x55b023dc1cc7 in sdhci_write ../../hw/sd/sdhci.c:1216
> > >     #2 0x55b024521e01 in memory_region_write_accessor ../../softmmu/memory.c:492
> > >     #3 0x55b0245222ab in access_with_adjusted_size ../../softmmu/memory.c:554
> > >     #4 0x55b02452ff15 in memory_region_dispatch_write
> > > ../../softmmu/memory.c:1514
> > >     #5 0x55b024568c67 in flatview_write_continue ../../softmmu/physmem.c:2814
> > >     #6 0x55b02456908d in flatview_write ../../softmmu/physmem.c:2856
> > >     #7 0x55b024569a74 in address_space_write ../../softmmu/physmem.c:2952
> > >     #8 0x55b02457a63c in qtest_process_command ../../softmmu/qtest.c:538
> > >     #9 0x55b02457ef97 in qtest_process_inbuf ../../softmmu/qtest.c:796
> > >     #10 0x55b02457f089 in qtest_read ../../softmmu/qtest.c:808
> > >     #11 0x55b0249d4372 in qemu_chr_be_write_impl ../../chardev/char.c:201
> > >     #12 0x55b0249d4414 in qemu_chr_be_write ../../chardev/char.c:213
> > >     #13 0x55b0249db586 in fd_chr_read ../../chardev/char-fd.c:72
> > >     #14 0x55b02466ba5b in qio_channel_fd_source_dispatch
> > > ../../io/channel-watch.c:84
> > >     #15 0x7f88093af0ae in g_main_context_dispatch
> > > (/lib64/libglib-2.0.so.0+0x550ae)
> > >     #16 0x55b024c5ff14 in glib_pollfds_poll ../../util/main-loop.c:297
> > >     #17 0x55b024c600fa in os_host_main_loop_wait ../../util/main-loop.c:320
> > >     #18 0x55b024c603f3 in main_loop_wait ../../util/main-loop.c:596
> > >     #19 0x55b023fcca21 in qemu_main_loop ../../softmmu/runstate.c:726
> > >     #20 0x55b023679735 in qemu_main ../../softmmu/main.c:36
> > >     #21 0x55b023679766 in main ../../softmmu/main.c:45
> > >     #22 0x7f8808728f5f in __libc_start_call_main (/lib64/libc.so.6+0x40f5f)
> > >     #23 0x7f880872900f in __libc_start_main_impl (/lib64/libc.so.6+0x4100f)
> > >     #24 0x55b023679644 in _start (./qemu-system-x86_64+0x20f2644)
> > >

Regards,
Bin

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.