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

[Peter Maydell]

On Wed, 17 Aug 2022 at 11:48, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> 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.

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

Thanks for the reminder -- I'd forgotten I needed to respin.

-- PMM