[PATCH 0/2] pci *_by_mask() coverity fix

[PATCH 0/2] pci *_by_mask() coverity fix

[Peter Maydell]

This patchset fixes a Coverity nit relating to the
pci_set_*_by_mask() helper functions, where we might shift off the
end of a variable if the caller passes in a bogus mask argument.
Patch 2 is the coverity fix (adding an assert() to help Coverity
out a bit and to catch potential future actual bugs). Patch 1
removes the _get_ versions of the functions, because they've been
in the tree for a decade and never had any callers at any point
in those 10 years :-)

thanks
-- PMM

Peter Maydell (2):
  pci: Remove unused pci_get_*_by_mask() functions
  pci: Sanity check mask argument to pci_set_*_by_mask()

 include/hw/pci/pci.h | 48 +++++++++++++++-----------------------------
 1 file changed, 16 insertions(+), 32 deletions(-)

-- 
2.25.1

[PATCH 1/2] pci: Remove unused pci_get_*_by_mask() functions

[Peter Maydell]

The helper functions pci_get_{byte,word,long,quad}_by_mask()
were added in 2012 in commit c9f50cea70a1596. In the decade
since we have never added a single use of them.

The helpers clearly aren't that helpful, so drop them
rather than carrying around dead code.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/pci/pci.h | 28 ----------------------------
 1 file changed, 28 deletions(-)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index b54b6ef88fc..c79144bc5ef 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -692,13 +692,6 @@ pci_set_byte_by_mask(uint8_t *config, uint8_t mask, uint8_t reg)
     pci_set_byte(config, (~mask & val) | (mask & rval));
 }
 
-static inline uint8_t
-pci_get_byte_by_mask(uint8_t *config, uint8_t mask)
-{
-    uint8_t val = pci_get_byte(config);
-    return (val & mask) >> ctz32(mask);
-}
-
 static inline void
 pci_set_word_by_mask(uint8_t *config, uint16_t mask, uint16_t reg)
 {
@@ -707,13 +700,6 @@ pci_set_word_by_mask(uint8_t *config, uint16_t mask, uint16_t reg)
     pci_set_word(config, (~mask & val) | (mask & rval));
 }
 
-static inline uint16_t
-pci_get_word_by_mask(uint8_t *config, uint16_t mask)
-{
-    uint16_t val = pci_get_word(config);
-    return (val & mask) >> ctz32(mask);
-}
-
 static inline void
 pci_set_long_by_mask(uint8_t *config, uint32_t mask, uint32_t reg)
 {
@@ -722,13 +708,6 @@ pci_set_long_by_mask(uint8_t *config, uint32_t mask, uint32_t reg)
     pci_set_long(config, (~mask & val) | (mask & rval));
 }
 
-static inline uint32_t
-pci_get_long_by_mask(uint8_t *config, uint32_t mask)
-{
-    uint32_t val = pci_get_long(config);
-    return (val & mask) >> ctz32(mask);
-}
-
 static inline void
 pci_set_quad_by_mask(uint8_t *config, uint64_t mask, uint64_t reg)
 {
@@ -737,13 +716,6 @@ pci_set_quad_by_mask(uint8_t *config, uint64_t mask, uint64_t reg)
     pci_set_quad(config, (~mask & val) | (mask & rval));
 }
 
-static inline uint64_t
-pci_get_quad_by_mask(uint8_t *config, uint64_t mask)
-{
-    uint64_t val = pci_get_quad(config);
-    return (val & mask) >> ctz32(mask);
-}
-
 PCIDevice *pci_new_multifunction(int devfn, bool multifunction,
                                     const char *name);
 PCIDevice *pci_new(int devfn, const char *name);
-- 
2.25.1

Re: [PATCH 1/2] pci: Remove unused pci_get_*_by_mask() functions

[Richard Henderson]

On 7/26/22 09:32, Peter Maydell wrote:
> The helper functions pci_get_{byte,word,long,quad}_by_mask()
> were added in 2012 in commit c9f50cea70a1596. In the decade
> since we have never added a single use of them.
> 
> The helpers clearly aren't that helpful, so drop them
> rather than carrying around dead code.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   include/hw/pci/pci.h | 28 ----------------------------
>   1 file changed, 28 deletions(-)

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

r~

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

[Peter Maydell]

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

[Philippe Mathieu-Daudé via]

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>

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

[Richard Henderson]

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

[Peter Maydell]

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

[Michael S. Tsirkin]

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

[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