Re: [Qemu-devel] [PATCH] sd: Fix out-of-bounds assertions

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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.
>