[v1] pci *_by_mask() coverity fix

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

Posted by Peter Maydell 3 years, 6 months ago

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);
+    rval = reg << ctz32(mask);
     pci_set_byte(config, (~mask & val) | (mask & rval));
 }
 
@@ -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 & 0xffff);
+    rval = reg << ctz32(mask);
     pci_set_word(config, (~mask & val) | (mask & rval));
 }
 
@@ -704,7 +710,10 @@ static inline void
 pci_set_long_by_mask(uint8_t *config, uint32_t mask, uint32_t reg)
 {
     uint32_t val = pci_get_long(config);
-    uint32_t rval = reg << ctz32(mask);
+    uint32_t rval;
+
+    assert(mask);
+    rval = reg << ctz32(mask);
     pci_set_long(config, (~mask & val) | (mask & rval));
 }
 
@@ -712,7 +721,10 @@ static inline void
 pci_set_quad_by_mask(uint8_t *config, uint64_t mask, uint64_t reg)
 {
     uint64_t val = pci_get_quad(config);
-    uint64_t rval = reg << ctz32(mask);
+    uint64_t rval;
+
+    assert(mask);
+    rval = reg << ctz32(mask);
     pci_set_quad(config, (~mask & val) | (mask & rval));
 }
 
-- 
2.25.1

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

Posted by Richard Henderson 3 years, 6 months ago

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?

> @@ -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 & 0xffff);

Similarly.

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

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

Posted by Peter Maydell 3 years, 6 months ago

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

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

Posted by Michael S. Tsirkin 3 years, 5 months ago

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

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

Posted by Peter Maydell 3 years, 5 months ago

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

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

Posted by Philippe Mathieu-Daudé via 3 years, 6 months ago

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 <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(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>