| Message ID | 20220726163206.1780707-3-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pci *_by_mask() coverity fix | expand |
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>
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~
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
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 &?
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
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)); }
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(-)