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

[Daniel P . Berrangé]

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 <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()?
> 
> 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 :|