Due to an off-by-one error, the assert statements allow an
out-of-bounds array access.

Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
---
 hw/sd/sd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index aaab15f..818f86c 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -144,7 +144,7 @@ static const char *sd_state_name(enum SDCardStates state)
     if (state == sd_inactive_state) {
         return "inactive";
     }
-    assert(state <= ARRAY_SIZE(state_name));
+    assert(state < ARRAY_SIZE(state_name));
     return state_name[state];
 }
 
@@ -165,7 +165,7 @@ static const char *sd_response_name(sd_rsp_type_t rsp)
     if (rsp == sd_r1b) {
         rsp = sd_r1;
     }
-    assert(rsp <= ARRAY_SIZE(response_name));
+    assert(rsp < ARRAY_SIZE(response_name));
     return response_name[rsp];
 }
 
-- 
1.8.3.1

On Mon, Apr 8, 2019 at 9:51 PM Lidong Chen <lidong.chen@oracle.com> wrote:
>
> Due to an off-by-one error, the assert statements allow an
> out-of-bounds array access.
>
> Signed-off-by: Lidong Chen <lidong.chen@oracle.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  hw/sd/sd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index aaab15f..818f86c 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -144,7 +144,7 @@ static const char *sd_state_name(enum SDCardStates state)
>      if (state == sd_inactive_state) {
>          return "inactive";
>      }
> -    assert(state <= ARRAY_SIZE(state_name));
> +    assert(state < ARRAY_SIZE(state_name));
>      return state_name[state];
>  }
>
> @@ -165,7 +165,7 @@ static const char *sd_response_name(sd_rsp_type_t rsp)
>      if (rsp == sd_r1b) {
>          rsp = sd_r1;
>      }
> -    assert(rsp <= ARRAY_SIZE(response_name));
> +    assert(rsp < ARRAY_SIZE(response_name));
>      return response_name[rsp];
>  }
>
> --
> 1.8.3.1
>
>


-- 
Marc-André Lureau

Hi Lidong

On 4/8/19 9:04 PM, Lidong Chen wrote:
> Due to an off-by-one error, the assert statements allow an
> out-of-bounds array access.

... which can't happen. Thus harmless for 4.0.

I suppose this is a static analysis warning and you didn't triggered it
while tracing.

Thanks for cleaning this :)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
> ---
>  hw/sd/sd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index aaab15f..818f86c 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -144,7 +144,7 @@ static const char *sd_state_name(enum SDCardStates state)
>      if (state == sd_inactive_state) {
>          return "inactive";
>      }
> -    assert(state <= ARRAY_SIZE(state_name));
> +    assert(state < ARRAY_SIZE(state_name));
>      return state_name[state];
>  }
>  
> @@ -165,7 +165,7 @@ static const char *sd_response_name(sd_rsp_type_t rsp)
>      if (rsp == sd_r1b) {
>          rsp = sd_r1;
>      }
> -    assert(rsp <= ARRAY_SIZE(response_name));
> +    assert(rsp < ARRAY_SIZE(response_name));
>      return response_name[rsp];
>  }
>  
> 

Hi Philippe,

On 4/8/2019 2:27 PM, Philippe Mathieu-Daudé wrote:
> Hi Lidong
>
> On 4/8/19 9:04 PM, Lidong Chen wrote:
>> Due to an off-by-one error, the assert statements allow an
>> out-of-bounds array access.
> ... which can't happen. Thus harmless for 4.0.
>
> I suppose this is a static analysis warning and you didn't triggered it
> while tracing.

Right, it was found by the static analysis.  Thank you for your review.

Regards,

Lidong


>
> Thanks for cleaning this :)
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
>> Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
>> ---
>>   hw/sd/sd.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index aaab15f..818f86c 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -144,7 +144,7 @@ static const char *sd_state_name(enum SDCardStates state)
>>       if (state == sd_inactive_state) {
>>           return "inactive";
>>       }
>> -    assert(state <= ARRAY_SIZE(state_name));
>> +    assert(state < ARRAY_SIZE(state_name));
>>       return state_name[state];
>>   }
>>   
>> @@ -165,7 +165,7 @@ static const char *sd_response_name(sd_rsp_type_t rsp)
>>       if (rsp == sd_r1b) {
>>           rsp = sd_r1;
>>       }
>> -    assert(rsp <= ARRAY_SIZE(response_name));
>> +    assert(rsp < ARRAY_SIZE(response_name));
>>       return response_name[rsp];
>>   }
>>   
>>

Lidong Chen <lidong.chen@oracle.com> 于2019年4月9日周二 上午3:51写道：

> Due to an off-by-one error, the assert statements allow an
> out-of-bounds array access.
>
> Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
>

Reviewed-by: Li Qiang <liq3ea@gmail.com>


> ---
>  hw/sd/sd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index aaab15f..818f86c 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -144,7 +144,7 @@ static const char *sd_state_name(enum SDCardStates
> state)
>      if (state == sd_inactive_state) {
>          return "inactive";
>      }
> -    assert(state <= ARRAY_SIZE(state_name));
> +    assert(state < ARRAY_SIZE(state_name));
>      return state_name[state];
>  }
>
> @@ -165,7 +165,7 @@ static const char *sd_response_name(sd_rsp_type_t rsp)
>      if (rsp == sd_r1b) {
>          rsp = sd_r1;
>      }
> -    assert(rsp <= ARRAY_SIZE(response_name));
> +    assert(rsp < ARRAY_SIZE(response_name));
>      return response_name[rsp];
>  }
>
> --
> 1.8.3.1
>
>
>

Lidong Chen <lidong.chen@oracle.com> writes:

> Due to an off-by-one error, the assert statements allow an
> out-of-bounds array access.
>
> Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
> ---
>  hw/sd/sd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index aaab15f..818f86c 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -144,7 +144,7 @@ static const char *sd_state_name(enum SDCardStates state)
>      if (state == sd_inactive_state) {
>          return "inactive";
>      }
> -    assert(state <= ARRAY_SIZE(state_name));
> +    assert(state < ARRAY_SIZE(state_name));
>      return state_name[state];
>  }
>  
> @@ -165,7 +165,7 @@ static const char *sd_response_name(sd_rsp_type_t rsp)
>      if (rsp == sd_r1b) {
>          rsp = sd_r1;
>      }
> -    assert(rsp <= ARRAY_SIZE(response_name));
> +    assert(rsp < ARRAY_SIZE(response_name));
>      return response_name[rsp];
>  }

This is the second fix for this bug pattern in a fortnight.  Where's
one, there are more:

$ git-grep '<= ARRAY_SIZE'
hw/intc/arm_gicv3_cpuif.c:    assert(aprmax <= ARRAY_SIZE(cs->ich_apr[0]));
hw/intc/arm_gicv3_cpuif.c:    assert(aprmax <= ARRAY_SIZE(cs->ich_apr[0]));
hw/net/stellaris_enet.c:        if (s->tx_fifo_len + 4 <= ARRAY_SIZE(s->tx_fifo)) {
hw/sd/pxa2xx_mmci.c:        && s->tx_len <= ARRAY_SIZE(s->tx_fifo)
hw/sd/pxa2xx_mmci.c:        && s->rx_len <= ARRAY_SIZE(s->rx_fifo)
hw/sd/pxa2xx_mmci.c:        && s->resp_len <= ARRAY_SIZE(s->resp_fifo);
hw/sd/sd.c:    assert(state <= ARRAY_SIZE(state_name));
hw/sd/sd.c:    assert(rsp <= ARRAY_SIZE(response_name));
hw/usb/hcd-xhci.c:    assert(n <= ARRAY_SIZE(tmp));
target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {
target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {
target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {
target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {
target/ppc/kvm.c:           <= ARRAY_SIZE(hw_debug_points));
target/ppc/kvm.c:           <= ARRAY_SIZE(hw_debug_points));
target/ppc/kvm.c:    assert((nb_hw_breakpoint + nb_hw_watchpoint) <= ARRAY_SIZE(dbg->arch.bp));
tcg/tcg.c:    tcg_debug_assert(pi <= ARRAY_SIZE(op->args));
util/main-loop.c:    g_assert(n_poll_fds <= ARRAY_SIZE(poll_fds));
util/module.c:    assert(n_dirs <= ARRAY_SIZE(dirs));

Lidong Chen, would you like to have a look at these?

Cc'ing maintainers to help with further investigation.

>
> Lidong Chen <lidong.chen@oracle.com> writes:
>
> > Due to an off-by-one error, the assert statements allow an
> > out-of-bounds array access.
> >
> > Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
> > ---
> >  hw/sd/sd.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> > index aaab15f..818f86c 100644
> > --- a/hw/sd/sd.c
> > +++ b/hw/sd/sd.c
> > @@ -144,7 +144,7 @@ static const char *sd_state_name(enum SDCardStates state)
> >      if (state == sd_inactive_state) {
> >          return "inactive";
> >      }
> > -    assert(state <= ARRAY_SIZE(state_name));
> > +    assert(state < ARRAY_SIZE(state_name));
> >      return state_name[state];
> >  }
> >
> > @@ -165,7 +165,7 @@ static const char *sd_response_name(sd_rsp_type_t rsp)
> >      if (rsp == sd_r1b) {
> >          rsp = sd_r1;
> >      }
> > -    assert(rsp <= ARRAY_SIZE(response_name));
> > +    assert(rsp < ARRAY_SIZE(response_name));
> >      return response_name[rsp];
> >  }
>
> This is the second fix for this bug pattern in a fortnight.  Where's
> one, there are more:
>
> $ git-grep '<= ARRAY_SIZE'
> hw/intc/arm_gicv3_cpuif.c:    assert(aprmax <= ARRAY_SIZE(cs->ich_apr[0]));
> hw/intc/arm_gicv3_cpuif.c:    assert(aprmax <= ARRAY_SIZE(cs->ich_apr[0]));
> hw/net/stellaris_enet.c:        if (s->tx_fifo_len + 4 <= ARRAY_SIZE(s->tx_fifo)) {
> hw/sd/pxa2xx_mmci.c:        && s->tx_len <= ARRAY_SIZE(s->tx_fifo)
> hw/sd/pxa2xx_mmci.c:        && s->rx_len <= ARRAY_SIZE(s->rx_fifo)
> hw/sd/pxa2xx_mmci.c:        && s->resp_len <= ARRAY_SIZE(s->resp_fifo);
> hw/sd/sd.c:    assert(state <= ARRAY_SIZE(state_name));
> hw/sd/sd.c:    assert(rsp <= ARRAY_SIZE(response_name));
> hw/usb/hcd-xhci.c:    assert(n <= ARRAY_SIZE(tmp));

> target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {
> target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {
> target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {
> target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {

The last four items are OK as they are. The variable multiple_regs is, in fact,
an array of 9 int constants:

static const int multiple_regs[] = { 16, 17, 18, 19, 20, 21, 22, 23, 30 };

ARRAY_SIZE (multiple_regs) will always be equal to 9.

The variable base_reglist (that is checked to be > 0 and <=9) is used
in succeeding lines like this:

        for (i = 0; i < base_reglist; i++) {
            do_sw(env, addr, env->active_tc.gpr[multiple_regs[i]], mem_idx,
                  GETPC());
            addr += 4;
        }

Therefore, the array multiple_regs will always be accessed within its bounds.

> target/ppc/kvm.c:           <= ARRAY_SIZE(hw_debug_points));
> target/ppc/kvm.c:           <= ARRAY_SIZE(hw_debug_points));
> target/ppc/kvm.c:    assert((nb_hw_breakpoint + nb_hw_watchpoint) <= ARRAY_SIZE(dbg->arch.bp));
> tcg/tcg.c:    tcg_debug_assert(pi <= ARRAY_SIZE(op->args));
> util/main-loop.c:    g_assert(n_poll_fds <= ARRAY_SIZE(poll_fds));
> util/module.c:    assert(n_dirs <= ARRAY_SIZE(dirs));
>
> Lidong Chen, would you like to have a look at these?
>
> Cc'ing maintainers to help with further investigation.
>

Thank you for bringing this to our attention, Markus. And thanks to Lidong too.

Aleksandar

P. S. Shouldn't perhaps our macro ARRAY_SIZE() be renamed to
NUMBER_OF_ELEMENTS()?

________________________________________
From: Markus Armbruster <armbru@redhat.com>
Sent: Tuesday, April 9, 2019 7:51:51 AM
To: Lidong Chen
Cc: qemu-devel@nongnu.org; darren.kenny@oracle.com; f4bug@amsat.org; Peter Maydell; Jason Wang; Andrzej Zaborowski; Gerd Hoffmann; Aurelien Jarno; Aleksandar Markovic; Aleksandar Rikalo; David Gibson; Richard Henderson; Paolo Bonzini
Subject: Re: [Qemu-devel] [PATCH] sd: Fix out-of-bounds assertions

Lidong Chen <lidong.chen@oracle.com> writes:

> Due to an off-by-one error, the assert statements allow an
> out-of-bounds array access.
>
> Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
> ---
>  hw/sd/sd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index aaab15f..818f86c 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -144,7 +144,7 @@ static const char *sd_state_name(enum SDCardStates state)
>      if (state == sd_inactive_state) {
>          return "inactive";
>      }
> -    assert(state <= ARRAY_SIZE(state_name));
> +    assert(state < ARRAY_SIZE(state_name));
>      return state_name[state];
>  }
>
> @@ -165,7 +165,7 @@ static const char *sd_response_name(sd_rsp_type_t rsp)
>      if (rsp == sd_r1b) {
>          rsp = sd_r1;
>      }
> -    assert(rsp <= ARRAY_SIZE(response_name));
> +    assert(rsp < ARRAY_SIZE(response_name));
>      return response_name[rsp];
>  }

This is the second fix for this bug pattern in a fortnight.  Where's
one, there are more:

$ git-grep '<= ARRAY_SIZE'
hw/intc/arm_gicv3_cpuif.c:    assert(aprmax <= ARRAY_SIZE(cs->ich_apr[0]));
hw/intc/arm_gicv3_cpuif.c:    assert(aprmax <= ARRAY_SIZE(cs->ich_apr[0]));
hw/net/stellaris_enet.c:        if (s->tx_fifo_len + 4 <= ARRAY_SIZE(s->tx_fifo)) {
hw/sd/pxa2xx_mmci.c:        && s->tx_len <= ARRAY_SIZE(s->tx_fifo)
hw/sd/pxa2xx_mmci.c:        && s->rx_len <= ARRAY_SIZE(s->rx_fifo)
hw/sd/pxa2xx_mmci.c:        && s->resp_len <= ARRAY_SIZE(s->resp_fifo);
hw/sd/sd.c:    assert(state <= ARRAY_SIZE(state_name));
hw/sd/sd.c:    assert(rsp <= ARRAY_SIZE(response_name));
hw/usb/hcd-xhci.c:    assert(n <= ARRAY_SIZE(tmp));
target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {
target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {
target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {
target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {
target/ppc/kvm.c:           <= ARRAY_SIZE(hw_debug_points));
target/ppc/kvm.c:           <= ARRAY_SIZE(hw_debug_points));
target/ppc/kvm.c:    assert((nb_hw_breakpoint + nb_hw_watchpoint) <= ARRAY_SIZE(dbg->arch.bp));
tcg/tcg.c:    tcg_debug_assert(pi <= ARRAY_SIZE(op->args));
util/main-loop.c:    g_assert(n_poll_fds <= ARRAY_SIZE(poll_fds));
util/module.c:    assert(n_dirs <= ARRAY_SIZE(dirs));

Lidong Chen, would you like to have a look at these?

Cc'ing maintainers to help with further investigation.

On 4/9/19 10:59 AM, Aleksandar Markovic wrote:
>>
>> Lidong Chen <lidong.chen@oracle.com> writes:
>>
>>> Due to an off-by-one error, the assert statements allow an
>>> out-of-bounds array access.
>>>
>>> Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
>>> ---
>>>  hw/sd/sd.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>> index aaab15f..818f86c 100644
>>> --- a/hw/sd/sd.c
>>> +++ b/hw/sd/sd.c
>>> @@ -144,7 +144,7 @@ static const char *sd_state_name(enum SDCardStates state)
>>>      if (state == sd_inactive_state) {
>>>          return "inactive";
>>>      }
>>> -    assert(state <= ARRAY_SIZE(state_name));
>>> +    assert(state < ARRAY_SIZE(state_name));
>>>      return state_name[state];
>>>  }
>>>
>>> @@ -165,7 +165,7 @@ static const char *sd_response_name(sd_rsp_type_t rsp)
>>>      if (rsp == sd_r1b) {
>>>          rsp = sd_r1;
>>>      }
>>> -    assert(rsp <= ARRAY_SIZE(response_name));
>>> +    assert(rsp < ARRAY_SIZE(response_name));
>>>      return response_name[rsp];
>>>  }
>>
>> This is the second fix for this bug pattern in a fortnight.  Where's
>> one, there are more:
>>
>> $ git-grep '<= ARRAY_SIZE'
>> hw/intc/arm_gicv3_cpuif.c:    assert(aprmax <= ARRAY_SIZE(cs->ich_apr[0]));
>> hw/intc/arm_gicv3_cpuif.c:    assert(aprmax <= ARRAY_SIZE(cs->ich_apr[0]));
>> hw/net/stellaris_enet.c:        if (s->tx_fifo_len + 4 <= ARRAY_SIZE(s->tx_fifo)) {
>> hw/sd/pxa2xx_mmci.c:        && s->tx_len <= ARRAY_SIZE(s->tx_fifo)
>> hw/sd/pxa2xx_mmci.c:        && s->rx_len <= ARRAY_SIZE(s->rx_fifo)
>> hw/sd/pxa2xx_mmci.c:        && s->resp_len <= ARRAY_SIZE(s->resp_fifo);
>> hw/sd/sd.c:    assert(state <= ARRAY_SIZE(state_name));
>> hw/sd/sd.c:    assert(rsp <= ARRAY_SIZE(response_name));
>> hw/usb/hcd-xhci.c:    assert(n <= ARRAY_SIZE(tmp));
> 
>> target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {
>> target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {
>> target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {
>> target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {
> 
> The last four items are OK as they are. The variable multiple_regs is, in fact,
> an array of 9 int constants:
> 
> static const int multiple_regs[] = { 16, 17, 18, 19, 20, 21, 22, 23, 30 };
> 
> ARRAY_SIZE (multiple_regs) will always be equal to 9.
> 
> The variable base_reglist (that is checked to be > 0 and <=9) is used
> in succeeding lines like this:
> 
>         for (i = 0; i < base_reglist; i++) {
>             do_sw(env, addr, env->active_tc.gpr[multiple_regs[i]], mem_idx,
>                   GETPC());
>             addr += 4;
>         }
> 
> Therefore, the array multiple_regs will always be accessed within its bounds.
> 
>> target/ppc/kvm.c:           <= ARRAY_SIZE(hw_debug_points));
>> target/ppc/kvm.c:           <= ARRAY_SIZE(hw_debug_points));
>> target/ppc/kvm.c:    assert((nb_hw_breakpoint + nb_hw_watchpoint) <= ARRAY_SIZE(dbg->arch.bp));
>> tcg/tcg.c:    tcg_debug_assert(pi <= ARRAY_SIZE(op->args));
>> util/main-loop.c:    g_assert(n_poll_fds <= ARRAY_SIZE(poll_fds));
>> util/module.c:    assert(n_dirs <= ARRAY_SIZE(dirs));
>>
>> Lidong Chen, would you like to have a look at these?
>>
>> Cc'ing maintainers to help with further investigation.
>>
> 
> Thank you for bringing this to our attention, Markus. And thanks to Lidong too.
> 
> Aleksandar
> 
> P. S. Shouldn't perhaps our macro ARRAY_SIZE() be renamed to
> NUMBER_OF_ELEMENTS()?

I remember this post from Daniel where he suggests sticking to GLib
G_N_ELEMENTS() (which looks similar to your suggestion):
https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg02676.html

$ git grep G_N_ELEMENTS|wc -l
125
$ git grep ARRAY_SIZE|wc -l
939

Now it is not obvious to me to understand which GLib API we are
encouraged to use and which ones we shouldn't.

> ________________________________________
> From: Markus Armbruster <armbru@redhat.com>
> Sent: Tuesday, April 9, 2019 7:51:51 AM
> To: Lidong Chen
> Cc: qemu-devel@nongnu.org; darren.kenny@oracle.com; f4bug@amsat.org; Peter Maydell; Jason Wang; Andrzej Zaborowski; Gerd Hoffmann; Aurelien Jarno; Aleksandar Markovic; Aleksandar Rikalo; David Gibson; Richard Henderson; Paolo Bonzini
> Subject: Re: [Qemu-devel] [PATCH] sd: Fix out-of-bounds assertions
> 
> Lidong Chen <lidong.chen@oracle.com> writes:
> 
>> Due to an off-by-one error, the assert statements allow an
>> out-of-bounds array access.
>>
>> Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
>> ---
>>  hw/sd/sd.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index aaab15f..818f86c 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -144,7 +144,7 @@ static const char *sd_state_name(enum SDCardStates state)
>>      if (state == sd_inactive_state) {
>>          return "inactive";
>>      }
>> -    assert(state <= ARRAY_SIZE(state_name));
>> +    assert(state < ARRAY_SIZE(state_name));
>>      return state_name[state];
>>  }
>>
>> @@ -165,7 +165,7 @@ static const char *sd_response_name(sd_rsp_type_t rsp)
>>      if (rsp == sd_r1b) {
>>          rsp = sd_r1;
>>      }
>> -    assert(rsp <= ARRAY_SIZE(response_name));
>> +    assert(rsp < ARRAY_SIZE(response_name));
>>      return response_name[rsp];
>>  }
> 
> This is the second fix for this bug pattern in a fortnight.  Where's
> one, there are more:
> 
> $ git-grep '<= ARRAY_SIZE'
> hw/intc/arm_gicv3_cpuif.c:    assert(aprmax <= ARRAY_SIZE(cs->ich_apr[0]));
> hw/intc/arm_gicv3_cpuif.c:    assert(aprmax <= ARRAY_SIZE(cs->ich_apr[0]));
> hw/net/stellaris_enet.c:        if (s->tx_fifo_len + 4 <= ARRAY_SIZE(s->tx_fifo)) {
> hw/sd/pxa2xx_mmci.c:        && s->tx_len <= ARRAY_SIZE(s->tx_fifo)
> hw/sd/pxa2xx_mmci.c:        && s->rx_len <= ARRAY_SIZE(s->rx_fifo)
> hw/sd/pxa2xx_mmci.c:        && s->resp_len <= ARRAY_SIZE(s->resp_fifo);
> hw/sd/sd.c:    assert(state <= ARRAY_SIZE(state_name));
> hw/sd/sd.c:    assert(rsp <= ARRAY_SIZE(response_name));
> hw/usb/hcd-xhci.c:    assert(n <= ARRAY_SIZE(tmp));
> target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {
> target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {
> target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {
> target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {
> target/ppc/kvm.c:           <= ARRAY_SIZE(hw_debug_points));
> target/ppc/kvm.c:           <= ARRAY_SIZE(hw_debug_points));
> target/ppc/kvm.c:    assert((nb_hw_breakpoint + nb_hw_watchpoint) <= ARRAY_SIZE(dbg->arch.bp));
> tcg/tcg.c:    tcg_debug_assert(pi <= ARRAY_SIZE(op->args));
> util/main-loop.c:    g_assert(n_poll_fds <= ARRAY_SIZE(poll_fds));
> util/module.c:    assert(n_dirs <= ARRAY_SIZE(dirs));
> 
> Lidong Chen, would you like to have a look at these?
> 
> Cc'ing maintainers to help with further investigation.
> 

On Tue, Apr 09, 2019 at 11:37:44AM +0200, Philippe Mathieu-Daudé wrote:
> On 4/9/19 10:59 AM, Aleksandar Markovic wrote:
> >>
> >> Lidong Chen <lidong.chen@oracle.com> writes:
> >>
> >>> Due to an off-by-one error, the assert statements allow an
> >>> out-of-bounds array access.
> >>>
> >>> Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
> >>> ---
> >>>  hw/sd/sd.c | 4 ++--
> >>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> >>> index aaab15f..818f86c 100644
> >>> --- a/hw/sd/sd.c
> >>> +++ b/hw/sd/sd.c
> >>> @@ -144,7 +144,7 @@ static const char *sd_state_name(enum SDCardStates state)
> >>>      if (state == sd_inactive_state) {
> >>>          return "inactive";
> >>>      }
> >>> -    assert(state <= ARRAY_SIZE(state_name));
> >>> +    assert(state < ARRAY_SIZE(state_name));
> >>>      return state_name[state];
> >>>  }
> >>>
> >>> @@ -165,7 +165,7 @@ static const char *sd_response_name(sd_rsp_type_t rsp)
> >>>      if (rsp == sd_r1b) {
> >>>          rsp = sd_r1;
> >>>      }
> >>> -    assert(rsp <= ARRAY_SIZE(response_name));
> >>> +    assert(rsp < ARRAY_SIZE(response_name));
> >>>      return response_name[rsp];
> >>>  }
> >>
> >> This is the second fix for this bug pattern in a fortnight.  Where's
> >> one, there are more:
> >>
> >> $ git-grep '<= ARRAY_SIZE'
> >> hw/intc/arm_gicv3_cpuif.c:    assert(aprmax <= ARRAY_SIZE(cs->ich_apr[0]));
> >> hw/intc/arm_gicv3_cpuif.c:    assert(aprmax <= ARRAY_SIZE(cs->ich_apr[0]));
> >> hw/net/stellaris_enet.c:        if (s->tx_fifo_len + 4 <= ARRAY_SIZE(s->tx_fifo)) {
> >> hw/sd/pxa2xx_mmci.c:        && s->tx_len <= ARRAY_SIZE(s->tx_fifo)
> >> hw/sd/pxa2xx_mmci.c:        && s->rx_len <= ARRAY_SIZE(s->rx_fifo)
> >> hw/sd/pxa2xx_mmci.c:        && s->resp_len <= ARRAY_SIZE(s->resp_fifo);
> >> hw/sd/sd.c:    assert(state <= ARRAY_SIZE(state_name));
> >> hw/sd/sd.c:    assert(rsp <= ARRAY_SIZE(response_name));
> >> hw/usb/hcd-xhci.c:    assert(n <= ARRAY_SIZE(tmp));
> > 
> >> target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {
> >> target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {
> >> target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {
> >> target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {
> > 
> > The last four items are OK as they are. The variable multiple_regs is, in fact,
> > an array of 9 int constants:
> > 
> > static const int multiple_regs[] = { 16, 17, 18, 19, 20, 21, 22, 23, 30 };
> > 
> > ARRAY_SIZE (multiple_regs) will always be equal to 9.
> > 
> > The variable base_reglist (that is checked to be > 0 and <=9) is used
> > in succeeding lines like this:
> > 
> >         for (i = 0; i < base_reglist; i++) {
> >             do_sw(env, addr, env->active_tc.gpr[multiple_regs[i]], mem_idx,
> >                   GETPC());
> >             addr += 4;
> >         }
> > 
> > Therefore, the array multiple_regs will always be accessed within its bounds.
> > 
> >> target/ppc/kvm.c:           <= ARRAY_SIZE(hw_debug_points));
> >> target/ppc/kvm.c:           <= ARRAY_SIZE(hw_debug_points));
> >> target/ppc/kvm.c:    assert((nb_hw_breakpoint + nb_hw_watchpoint) <= ARRAY_SIZE(dbg->arch.bp));
> >> tcg/tcg.c:    tcg_debug_assert(pi <= ARRAY_SIZE(op->args));
> >> util/main-loop.c:    g_assert(n_poll_fds <= ARRAY_SIZE(poll_fds));
> >> util/module.c:    assert(n_dirs <= ARRAY_SIZE(dirs));
> >>
> >> Lidong Chen, would you like to have a look at these?
> >>
> >> Cc'ing maintainers to help with further investigation.
> >>
> > 
> > Thank you for bringing this to our attention, Markus. And thanks to Lidong too.
> > 
> > Aleksandar
> > 
> > P. S. Shouldn't perhaps our macro ARRAY_SIZE() be renamed to
> > NUMBER_OF_ELEMENTS()?
> 
> I remember this post from Daniel where he suggests sticking to GLib
> G_N_ELEMENTS() (which looks similar to your suggestion):
> https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg02676.html
> 
> $ git grep G_N_ELEMENTS|wc -l
> 125
> $ git grep ARRAY_SIZE|wc -l
> 939
> 
> Now it is not obvious to me to understand which GLib API we are
> encouraged to use and which ones we shouldn't.

We have a bunch of duplication that is essentially a historical
artifact from before we used GLib in QEMU. IMHO, if GLib provides
something that is equivalent with no functional downside that
matters to QEMU, then there's no reason to keep QEMU's duplicate.

IOW, I would always prefer GLib unless there's a compelling reason
not to in order to minimize what we maintain ourselves


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Apr 09, 2019 at 11:37:44AM +0200, Philippe Mathieu-Daudé wrote:
[...]
>> I remember this post from Daniel where he suggests sticking to GLib
>> G_N_ELEMENTS() (which looks similar to your suggestion):
>> https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg02676.html
>> 
>> $ git grep G_N_ELEMENTS|wc -l
>> 125
>> $ git grep ARRAY_SIZE|wc -l
>> 939
>> 
>> Now it is not obvious to me to understand which GLib API we are
>> encouraged to use and which ones we shouldn't.
>
> We have a bunch of duplication that is essentially a historical
> artifact from before we used GLib in QEMU. IMHO, if GLib provides
> something that is equivalent with no functional downside that
> matters to QEMU, then there's no reason to keep QEMU's duplicate.
>
> IOW, I would always prefer GLib unless there's a compelling reason
> not to in order to minimize what we maintain ourselves

Without a tree-wide sweep, we won't ever stop maintaining ARRAY_SIZE().

As long as we maintain it anyway, I'd prefer it over G_N_ELEMENTS()
myself, because I consider latter name in poor taste: elements of
*what*?

On Thu, Apr 11, 2019 at 02:20:27PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Tue, Apr 09, 2019 at 11:37:44AM +0200, Philippe Mathieu-Daudé wrote:
> [...]
> >> I remember this post from Daniel where he suggests sticking to GLib
> >> G_N_ELEMENTS() (which looks similar to your suggestion):
> >> https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg02676.html
> >> 
> >> $ git grep G_N_ELEMENTS|wc -l
> >> 125
> >> $ git grep ARRAY_SIZE|wc -l
> >> 939
> >> 
> >> Now it is not obvious to me to understand which GLib API we are
> >> encouraged to use and which ones we shouldn't.
> >
> > We have a bunch of duplication that is essentially a historical
> > artifact from before we used GLib in QEMU. IMHO, if GLib provides
> > something that is equivalent with no functional downside that
> > matters to QEMU, then there's no reason to keep QEMU's duplicate.
> >
> > IOW, I would always prefer GLib unless there's a compelling reason
> > not to in order to minimize what we maintain ourselves
> 
> Without a tree-wide sweep, we won't ever stop maintaining ARRAY_SIZE().

In this case it is a pretty trivial search & replace to do. The main
hard bit would be syncing with various maintainer trees. Global cleanups
are more work if you need to feed patches in via several different trees,
as opposed to one tree-wide series.

> As long as we maintain it anyway, I'd prefer it over G_N_ELEMENTS()
> myself, because I consider latter name in poor taste: elements of
> *what*?

elements of the variable that you are passing into the macro.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Apr 11, 2019 at 02:20:27PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > On Tue, Apr 09, 2019 at 11:37:44AM +0200, Philippe Mathieu-Daudé wrote:
>> [...]
>> >> I remember this post from Daniel where he suggests sticking to GLib
>> >> G_N_ELEMENTS() (which looks similar to your suggestion):
>> >> https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg02676.html
>> >> 
>> >> $ git grep G_N_ELEMENTS|wc -l
>> >> 125
>> >> $ git grep ARRAY_SIZE|wc -l
>> >> 939
>> >> 
>> >> Now it is not obvious to me to understand which GLib API we are
>> >> encouraged to use and which ones we shouldn't.
>> >
>> > We have a bunch of duplication that is essentially a historical
>> > artifact from before we used GLib in QEMU. IMHO, if GLib provides
>> > something that is equivalent with no functional downside that
>> > matters to QEMU, then there's no reason to keep QEMU's duplicate.
>> >
>> > IOW, I would always prefer GLib unless there's a compelling reason
>> > not to in order to minimize what we maintain ourselves
>> 
>> Without a tree-wide sweep, we won't ever stop maintaining ARRAY_SIZE().
>
> In this case it is a pretty trivial search & replace to do. The main
> hard bit would be syncing with various maintainer trees. Global cleanups
> are more work if you need to feed patches in via several different trees,
> as opposed to one tree-wide series.

Yes, it's a hassle.  I doubt it's worthwhile.  Anyway, unless somebody
does this work, ARRAY_SIZE() will surely stay.

>> As long as we maintain it anyway, I'd prefer it over G_N_ELEMENTS()
>> myself, because I consider latter name in poor taste: elements of
>> *what*?
>
> elements of the variable that you are passing into the macro.

I pass a GSList * variable.

Markus wrote:

> This is the second fix for this bug pattern in a fortnight.  Where's
> one, there are more:
> 
> $ git-grep '<= ARRAY_SIZE'
> hw/intc/arm_gicv3_cpuif.c:    assert(aprmax <= ARRAY_SIZE(cs->ich_apr[0]));
> hw/intc/arm_gicv3_cpuif.c:    assert(aprmax <= ARRAY_SIZE(cs->ich_apr[0]));
> hw/net/stellaris_enet.c:        if (s->tx_fifo_len + 4 <= ARRAY_SIZE(s->tx_fifo)) {
> hw/sd/pxa2xx_mmci.c:        && s->tx_len <= ARRAY_SIZE(s->tx_fifo)
> hw/sd/pxa2xx_mmci.c:        && s->rx_len <= ARRAY_SIZE(s->rx_fifo)
> hw/sd/pxa2xx_mmci.c:        && s->resp_len <= ARRAY_SIZE(s->resp_fifo);
> hw/sd/sd.c:    assert(state <= ARRAY_SIZE(state_name));
> hw/sd/sd.c:    assert(rsp <= ARRAY_SIZE(response_name));
> hw/usb/hcd-xhci.c:    assert(n <= ARRAY_SIZE(tmp));
> target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {
> target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {
> target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {
> target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {
> target/ppc/kvm.c:           <= ARRAY_SIZE(hw_debug_points));
> target/ppc/kvm.c:           <= ARRAY_SIZE(hw_debug_points));
> target/ppc/kvm.c:    assert((nb_hw_breakpoint + nb_hw_watchpoint) <= ARRAY_SIZE(dbg->arch.bp));
> tcg/tcg.c:    tcg_debug_assert(pi <= ARRAY_SIZE(op->args));
> util/main-loop.c:    g_assert(n_poll_fds <= ARRAY_SIZE(poll_fds));
> util/module.c:    assert(n_dirs <= ARRAY_SIZE(dirs));

There could be even more:

$ git grep '> ARRAY_SIZE'
hw/display/ssd0323.c:    if (s->cmd_len > ARRAY_SIZE(s->cmd_data)) {
hw/display/vmware_vga.c:                || SVGA_BITMAP_SIZE(x, y) > ARRAY_SIZE(cursor.mask)
hw/display/vmware_vga.c:                    > ARRAY_SIZE(cursor.image)) {
hw/dma/xlnx-zdma.c:        len = src_size > ARRAY_SIZE(s->buf) ? ARRAY_SIZE(s->buf) : src_size;
hw/net/stellaris_enet.c:    if (s->np > ARRAY_SIZE(s->rx)) {
hw/net/stellaris_enet.c:        if (s->rx[i].len > ARRAY_SIZE(s->rx[i].data)) {
hw/net/stellaris_enet.c:    if (s->rx_fifo_offset > ARRAY_SIZE(s->rx[0].data) - 4) {
hw/net/stellaris_enet.c:    if (s->tx_fifo_len > ARRAY_SIZE(s->tx_fifo)) {
hw/scsi/mptsas.c:    ((s)->name##_head > ARRAY_SIZE((s)->name) ||         \
hw/scsi/mptsas.c:     (s)->name##_tail > ARRAY_SIZE((s)->name))
hw/scsi/mptsas.c:        s->doorbell_cnt > ARRAY_SIZE(s->doorbell_msg) ||
hw/scsi/mptsas.c:        s->doorbell_reply_size > ARRAY_SIZE(s->doorbell_reply) ||
hw/sd/ssi-sd.c:        (!s->stopping && s->arglen > ARRAY_SIZE(s->response)))) {
hw/usb/dev-mtp.c:            if (cmd.argc > ARRAY_SIZE(cmd.argv)) {
linux-user/syscall.c:    if (nargs[num] > ARRAY_SIZE(a)) {
target/sh4/translate.c:    if (max_insns > ARRAY_SIZE(insns)) {

CC-ing additional maintainers.

Aleksandar

On Tue, 9 Apr 2019 at 06:51, Markus Armbruster <armbru@redhat.com> wrote:
> $ git-grep '<= ARRAY_SIZE'

Almost all of these are OK because they're the pattern of
checking a loop upper bound before doing a loop.

> hw/intc/arm_gicv3_cpuif.c:    assert(aprmax <= ARRAY_SIZE(cs->ich_apr[0]));
> hw/intc/arm_gicv3_cpuif.c:    assert(aprmax <= ARRAY_SIZE(cs->ich_apr[0]));

These are OK -- we're checking that the upper bound of
a loop (i = 0; i < aprmax; i++) is not greater than the worst
possible upper bound imposed by the array size.

> hw/net/stellaris_enet.c:        if (s->tx_fifo_len + 4 <= ARRAY_SIZE(s->tx_fifo)) {

This is OK, because we're checking that we can
store to the 4 entries starting at s->tx_fifo[s->tx_fifo_len].

> hw/sd/pxa2xx_mmci.c:        && s->tx_len <= ARRAY_SIZE(s->tx_fifo)
> hw/sd/pxa2xx_mmci.c:        && s->rx_len <= ARRAY_SIZE(s->rx_fifo)
> hw/sd/pxa2xx_mmci.c:        && s->resp_len <= ARRAY_SIZE(s->resp_fifo);

These are OK beacuse they're checking the number of bytes
held in the array, not an index.

> hw/sd/sd.c:    assert(state <= ARRAY_SIZE(state_name));
> hw/sd/sd.c:    assert(rsp <= ARRAY_SIZE(response_name));

These are bugs, as Philippe says.

> hw/usb/hcd-xhci.c:    assert(n <= ARRAY_SIZE(tmp));

This is another check of a loop upper bound, so it's OK.

> target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {
> target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {
> target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {
> target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {

OK, as Aleksandar has said.

> target/ppc/kvm.c:           <= ARRAY_SIZE(hw_debug_points));

Loop upper bound check, OK.

> target/ppc/kvm.c:           <= ARRAY_SIZE(hw_debug_points));

Ditto.

> target/ppc/kvm.c:    assert((nb_hw_breakpoint + nb_hw_watchpoint) <= ARRAY_SIZE(dbg->arch.bp));

Ditto.

> tcg/tcg.c:    tcg_debug_assert(pi <= ARRAY_SIZE(op->args));

This is doing a check after a series of "op->args[pi++]" actions,
so the sense of the test is correct and the question is just
whether it would be better to do the assert before every
array access (which would be pretty expensive and in any
case wouldn't affect production systems where tcg_debug_assert()
is a no-op.)

> util/main-loop.c:    g_assert(n_poll_fds <= ARRAY_SIZE(poll_fds));

This one seems to be doing an incorrect check, because
we are about to do accesses to poll_fds[n_poll_fds + i]
for i in [0, w->num), so the assert should be checking
"n_poll_fds + w->num <= ARRAY_SIZE(poll_fds)" I think.

> util/module.c:    assert(n_dirs <= ARRAY_SIZE(dirs));

Loop upper bound check, OK.

thanks
-- PMM

On 09/04/2019 06:51, Markus Armbruster wrote:
> Lidong Chen <lidong.chen@oracle.com> writes:
>
>> Due to an off-by-one error, the assert statements allow an
>> out-of-bounds array access.
>>
>> Signed-off-by: Lidong Chen <lidong.chen@oracle.com>


Reviewed-by: Liam Merwick <liam.merwick@oracle.com>


>> ---
>>   hw/sd/sd.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index aaab15f..818f86c 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -144,7 +144,7 @@ static const char *sd_state_name(enum SDCardStates state)
>>       if (state == sd_inactive_state) {
>>           return "inactive";
>>       }
>> -    assert(state <= ARRAY_SIZE(state_name));
>> +    assert(state < ARRAY_SIZE(state_name));
>>       return state_name[state];
>>   }
>>   
>> @@ -165,7 +165,7 @@ static const char *sd_response_name(sd_rsp_type_t rsp)
>>       if (rsp == sd_r1b) {
>>           rsp = sd_r1;
>>       }
>> -    assert(rsp <= ARRAY_SIZE(response_name));
>> +    assert(rsp < ARRAY_SIZE(response_name));
>>       return response_name[rsp];
>>   }
> This is the second fix for this bug pattern in a fortnight.  Where's
> one, there are more:


As Lidong mentioned, an internal static analysis tool (Parfait) was used 
to catch these. Not every arch/board is compiled but I had eyeballed the 
code of most interest to me and they seemed fine (e.g. for array 
accesses, the subsequent loops used a less-than check)

However, this WIN32 code in util/main-loop.c seems wrong.

425     g_assert(n_poll_fds <= ARRAY_SIZE(poll_fds));
426
427     for (i = 0; i < w->num; i++) {
428         poll_fds[n_poll_fds + i].fd = (DWORD_PTR)w->events[i];
429         poll_fds[n_poll_fds + i].events = G_IO_IN;
430     }

Seems like this should be:

g_assert(n_poll_fds + w->num <= ARRAY_SIZE(poll_fds));

Otherwise, the rest seem fine.

Regards,

Liam


>
> $ git-grep '<= ARRAY_SIZE'
> hw/intc/arm_gicv3_cpuif.c:    assert(aprmax <= ARRAY_SIZE(cs->ich_apr[0]));
> hw/intc/arm_gicv3_cpuif.c:    assert(aprmax <= ARRAY_SIZE(cs->ich_apr[0]));
> hw/net/stellaris_enet.c:        if (s->tx_fifo_len + 4 <= ARRAY_SIZE(s->tx_fifo)) {
> hw/sd/pxa2xx_mmci.c:        && s->tx_len <= ARRAY_SIZE(s->tx_fifo)
> hw/sd/pxa2xx_mmci.c:        && s->rx_len <= ARRAY_SIZE(s->rx_fifo)
> hw/sd/pxa2xx_mmci.c:        && s->resp_len <= ARRAY_SIZE(s->resp_fifo);
> hw/sd/sd.c:    assert(state <= ARRAY_SIZE(state_name));
> hw/sd/sd.c:    assert(rsp <= ARRAY_SIZE(response_name));
> hw/usb/hcd-xhci.c:    assert(n <= ARRAY_SIZE(tmp));
> target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {
> target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {
> target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {
> target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= ARRAY_SIZE (multiple_regs)) {
> target/ppc/kvm.c:           <= ARRAY_SIZE(hw_debug_points));
> target/ppc/kvm.c:           <= ARRAY_SIZE(hw_debug_points));
> target/ppc/kvm.c:    assert((nb_hw_breakpoint + nb_hw_watchpoint) <= ARRAY_SIZE(dbg->arch.bp));
> tcg/tcg.c:    tcg_debug_assert(pi <= ARRAY_SIZE(op->args));
> util/main-loop.c:    g_assert(n_poll_fds <= ARRAY_SIZE(poll_fds));
> util/module.c:    assert(n_dirs <= ARRAY_SIZE(dirs));
>
> Lidong Chen, would you like to have a look at these?
>
> Cc'ing maintainers to help with further investigation.
>

Hi,

Thank you all for the reviews and comments! Since the fixes in sd.c have 
gone through the review, I can fix the issue in util/main-loop.c 
(mentioned in the reviews of Peter and Liam) in a separate patch.

Thanks,

Lidong

On 4/9/2019 3:39 AM, Liam Merwick wrote:
> On 09/04/2019 06:51, Markus Armbruster wrote:
>> Lidong Chen <lidong.chen@oracle.com> writes:
>>
>>> Due to an off-by-one error, the assert statements allow an
>>> out-of-bounds array access.
>>>
>>> Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
>
>
> Reviewed-by: Liam Merwick <liam.merwick@oracle.com>
>
>
>>> ---
>>>   hw/sd/sd.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>> index aaab15f..818f86c 100644
>>> --- a/hw/sd/sd.c
>>> +++ b/hw/sd/sd.c
>>> @@ -144,7 +144,7 @@ static const char *sd_state_name(enum 
>>> SDCardStates state)
>>>       if (state == sd_inactive_state) {
>>>           return "inactive";
>>>       }
>>> -    assert(state <= ARRAY_SIZE(state_name));
>>> +    assert(state < ARRAY_SIZE(state_name));
>>>       return state_name[state];
>>>   }
>>>   @@ -165,7 +165,7 @@ static const char 
>>> *sd_response_name(sd_rsp_type_t rsp)
>>>       if (rsp == sd_r1b) {
>>>           rsp = sd_r1;
>>>       }
>>> -    assert(rsp <= ARRAY_SIZE(response_name));
>>> +    assert(rsp < ARRAY_SIZE(response_name));
>>>       return response_name[rsp];
>>>   }
>> This is the second fix for this bug pattern in a fortnight. Where's
>> one, there are more:
>
>
> As Lidong mentioned, an internal static analysis tool (Parfait) was 
> used to catch these. Not every arch/board is compiled but I had 
> eyeballed the code of most interest to me and they seemed fine (e.g. 
> for array accesses, the subsequent loops used a less-than check)
>
> However, this WIN32 code in util/main-loop.c seems wrong.
>
> 425     g_assert(n_poll_fds <= ARRAY_SIZE(poll_fds));
> 426
> 427     for (i = 0; i < w->num; i++) {
> 428         poll_fds[n_poll_fds + i].fd = (DWORD_PTR)w->events[i];
> 429         poll_fds[n_poll_fds + i].events = G_IO_IN;
> 430     }
>
> Seems like this should be:
>
> g_assert(n_poll_fds + w->num <= ARRAY_SIZE(poll_fds));
>
> Otherwise, the rest seem fine.
>
> Regards,
>
> Liam
>
>
>>
>> $ git-grep '<= ARRAY_SIZE'
>> hw/intc/arm_gicv3_cpuif.c:    assert(aprmax <= 
>> ARRAY_SIZE(cs->ich_apr[0]));
>> hw/intc/arm_gicv3_cpuif.c:    assert(aprmax <= 
>> ARRAY_SIZE(cs->ich_apr[0]));
>> hw/net/stellaris_enet.c:        if (s->tx_fifo_len + 4 <= 
>> ARRAY_SIZE(s->tx_fifo)) {
>> hw/sd/pxa2xx_mmci.c:        && s->tx_len <= ARRAY_SIZE(s->tx_fifo)
>> hw/sd/pxa2xx_mmci.c:        && s->rx_len <= ARRAY_SIZE(s->rx_fifo)
>> hw/sd/pxa2xx_mmci.c:        && s->resp_len <= ARRAY_SIZE(s->resp_fifo);
>> hw/sd/sd.c:    assert(state <= ARRAY_SIZE(state_name));
>> hw/sd/sd.c:    assert(rsp <= ARRAY_SIZE(response_name));
>> hw/usb/hcd-xhci.c:    assert(n <= ARRAY_SIZE(tmp));
>> target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= 
>> ARRAY_SIZE (multiple_regs)) {
>> target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= 
>> ARRAY_SIZE (multiple_regs)) {
>> target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= 
>> ARRAY_SIZE (multiple_regs)) {
>> target/mips/op_helper.c:    if (base_reglist > 0 && base_reglist <= 
>> ARRAY_SIZE (multiple_regs)) {
>> target/ppc/kvm.c:           <= ARRAY_SIZE(hw_debug_points));
>> target/ppc/kvm.c:           <= ARRAY_SIZE(hw_debug_points));
>> target/ppc/kvm.c:    assert((nb_hw_breakpoint + nb_hw_watchpoint) <= 
>> ARRAY_SIZE(dbg->arch.bp));
>> tcg/tcg.c:    tcg_debug_assert(pi <= ARRAY_SIZE(op->args));
>> util/main-loop.c:    g_assert(n_poll_fds <= ARRAY_SIZE(poll_fds));
>> util/module.c:    assert(n_dirs <= ARRAY_SIZE(dirs));
>>
>> Lidong Chen, would you like to have a look at these?
>>
>> Cc'ing maintainers to help with further investigation.
>>
>