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

[Aleksandar Markovic]

>
> Lidong Chen <address@hidden> writes:
>
> > Due to an off-by-one error, the assert statements allow an
> > out-of-bounds array access.
> >
> > Signed-off-by: Lidong Chen <address@hidden>
> > ---
> >  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 <address@hidden>
Sent: Tuesday, April 9, 2019 7:51:51 AM
To: Lidong Chen
Cc: address@hidden; address@hidden; address@hidden; 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 <address@hidden> writes:

> Due to an off-by-one error, the assert statements allow an
> out-of-bounds array access.
>
> Signed-off-by: Lidong Chen <address@hidden>
> ---
>  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.