Re: [Xen-devel] [PATCH] arm: Coverity 1469342 correct find_*_bit() functions use

[Artem Mygaiev <artem_mygaiev@xxxxxxxx>]

Hi Julien

On 24.05.18 16:49, Julien Grall wrote:
> Hi Artem,
>
> Thank you for the report.
>
> On 24/05/18 14:20, Artem Mygaiev wrote:
> > vgic_vcpu_pending_irq() uses find_next_bit() library function with 
> > single 'unsigned long' variable, while it is designed to work with 
> > memory regions. Nothing wrong is happening since 'offset' is set to 0 
> > (other values could lead to memory corruption), but it would be more 
> > correct to use the find_first_bit() function instead.
> I don't understand the commit message. It is fine to use other offset 
> than 0 in find_next_bit as long as it is smaller than 32. There would be 
> no corruption happening.
> Furthermore, find_first_bit(&apr, 32, 0) and find_next_bit(&apr, 32) are 
> equivalent because the former is just a macro using the latter (see 
> include/asm-arm/arm64/bitops.h).
> So as it is the patch is not solving anything. However, I think this is 
> just a false positive. Coverity should be able to guess that it will not 
> go past the array (BITOP_WORD will turned into 0).
Absolutely agree with you. Probably my message was not clear enough - 
with this particular patch I am not trying to fix a memory corruption, 
there is no memory corruption in the code now. It is just the use of 
functions: find_first_bit() is a better fit since the 
vgic_vcpu_pending_irq() function does not need to go over memory region 
and checks only one 32-bit variable. I have mentioned Coverity issue 
here because this was a false positive detected after today's test run.
> > Coverity Scan issue 1469342
> For future reference, please use the tag: "Coverity-ID: 1469342".
Thanks, will do.

> > Signed-off-by: Artem Mygaiev <artem_mygaiev@xxxxxxxx>
> >
> > diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
> > index d831b35525..fd63906e9b 100644
> > --- a/xen/arch/arm/gic-vgic.c
> > +++ b/xen/arch/arm/gic-vgic.c
> > @@ -362,7 +362,7 @@ int vgic_vcpu_pending_irq(struct vcpu *v)
> >       ASSERT(v == current);
> >
> >       mask_priority = gic_hw_ops->read_vmcr_priority();
> > -    active_priority = find_next_bit(&apr, 32, 0);
> > +    active_priority = find_first_bit(&apr, 32);
> >
> >       spin_lock_irqsave(&v->arch.vgic.lock, flags);
> Cheers,
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel