> > Lidong Chen <lidong.c...@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.c...@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 <arm...@redhat.com> Sent: Tuesday, April 9, 2019 7:51:51 AM To: Lidong Chen Cc: qemu-devel@nongnu.org; darren.ke...@oracle.com; f4...@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.c...@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.c...@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.