Re: [PATCH 2/2] pci: Sanity check mask argument to pci_set_*_by_mask()
On Wed, 17 Aug 2022 at 11:48, Michael S. Tsirkin wrote:
>
> On Wed, Jul 27, 2022 at 11:26:15AM +0100, Peter Maydell wrote:
> > On Tue, 26 Jul 2022 at 23:30, Richard Henderson
> > 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
> > > > ---
> > > > 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
Re: [PATCH 2/2] pci: Sanity check mask argument to pci_set_*_by_mask()
On Wed, Jul 27, 2022 at 11:26:15AM +0100, Peter Maydell wrote:
> On Tue, 26 Jul 2022 at 23:30, Richard Henderson
> 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
> > > ---
> > > 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
Re: [PATCH 2/2] pci: Sanity check mask argument to pci_set_*_by_mask()
On Tue, 26 Jul 2022 at 23:30, Richard Henderson
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
> > ---
> > 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
Re: [PATCH 2/2] pci: Sanity check mask argument to pci_set_*_by_mask()
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
---
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?
@@ -696,7 +699,10 @@ static inline void
pci_set_word_by_mask(uint8_t *config, uint16_t mask, uint16_t reg)
{
uint16_t val = pci_get_word(config);
-uint16_t rval = reg << ctz32(mask);
+uint16_t rval;
+
+assert(mask & 0x);
Similarly.
Otherwise,
Reviewed-by: Richard Henderson
r~
Re: [PATCH 2/2] pci: Sanity check mask argument to pci_set_*_by_mask()
On 26/7/22 18: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 --- 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(-) Reviewed-by: Philippe Mathieu-Daudé
