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

[Peter Maydell]

On Tue, 9 Apr 2019 at 06:51, Markus Armbruster <address@hidden> 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