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

[Lidong Chen]

Hi,

Thank you all for the reviews and comments! Since the fixes in sd.c have 
gone through the review, I can fix the issue in util/main-loop.c 
(mentioned in the reviews of Peter and Liam) in a separate patch.

Thanks,

Lidong

On 4/9/2019 3:39 AM, Liam Merwick wrote:
> On 09/04/2019 06:51, Markus Armbruster 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>
>
>
> Reviewed-by: Liam Merwick <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:
>
>
> As Lidong mentioned, an internal static analysis tool (Parfait) was 
> used to catch these. Not every arch/board is compiled but I had 
> eyeballed the code of most interest to me and they seemed fine (e.g. 
> for array accesses, the subsequent loops used a less-than check)
>
> However, this WIN32 code in util/main-loop.c seems wrong.
>
> 425     g_assert(n_poll_fds <= ARRAY_SIZE(poll_fds));
> 426
> 427     for (i = 0; i < w->num; i++) {
> 428         poll_fds[n_poll_fds + i].fd = (DWORD_PTR)w->events[i];
> 429         poll_fds[n_poll_fds + i].events = G_IO_IN;
> 430     }
>
> Seems like this should be:
>
> g_assert(n_poll_fds + w->num <= ARRAY_SIZE(poll_fds));
>
> Otherwise, the rest seem fine.
>
> Regards,
>
> Liam
>
>
> > $ 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.