Re: [PATCH 2/2] pci: Sanity check mask argument to pci_set_*_by_mask()

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Jul 27, 2022 at 11:26:15AM +0100, Peter Maydell wrote:
> On Tue, 26 Jul 2022 at 23:30, Richard Henderson
> <richard.henderson@linaro.org> wrote:
> >
> > On 7/26/22 09:32, Peter Maydell wrote:
> > > Coverity complains that in functions like pci_set_word_by_mask()
> > > we might end up shifting by more than 31 bits. This is true,
> > > but only if the caller passes in a zero mask. Help Coverity out
> > > by asserting that the mask argument is valid.
> > >
> > > Fixes: CID 1487168
> > >
> > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > > ---
> > > Note that only 1 of these 4 functions is used, and that only
> > > in 2 places in the codebase. In both cases the mask argument
> > > is a compile-time constant.
> > > ---
> > >   include/hw/pci/pci.h | 20 ++++++++++++++++----
> > >   1 file changed, 16 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > > index c79144bc5ef..0392b947b8b 100644
> > > --- a/include/hw/pci/pci.h
> > > +++ b/include/hw/pci/pci.h
> > > @@ -688,7 +688,10 @@ static inline void
> > >   pci_set_byte_by_mask(uint8_t *config, uint8_t mask, uint8_t reg)
> > >   {
> > >       uint8_t val = pci_get_byte(config);
> > > -    uint8_t rval = reg << ctz32(mask);
> > > +    uint8_t rval;
> > > +
> > > +    assert(mask & 0xff);
> >
> > Why the and, especially considering the uint8_t type?
> 
> Oops, yes. I think I was mixing up prototypes and thought the
> mask was passed as a 32-bit value in both these functions.
> 
> -- PMM

Did you intend to send v2 of this without the &?

-- 
MST