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

[Richard Henderson <richard.henderson@linaro.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~