[PATCH] misc/pca9554: Fix check of pin range value in property accessors

[PATCH] misc/pca9554: Fix check of pin range value in property accessors

[Cédric Le Goater]

Coverity detected an "Integer handling" issue with the pin value :

  In expression "state >> pin", right shifting "state" by more than 7
  bits always yields zero.  The shift amount, "pin", is as much as 8.

In practice, this should not happen because the properties "pin8" and
above are not created. Nevertheless, fix the range to avoid this warning.

Fixes: CID 1534917
Fixes: de0c7d543bca ("misc: Add a pca9554 GPIO device model")
Cc: Glenn Miles <milesg@linux.vnet.ibm.com>
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 hw/misc/pca9554.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/misc/pca9554.c b/hw/misc/pca9554.c
index 778b32e4430a8b618322c26b1b185ed3ead97cc4..5e31696797d9564666ece6fe17737ee2a9733e96 100644
--- a/hw/misc/pca9554.c
+++ b/hw/misc/pca9554.c
@@ -160,7 +160,7 @@ static void pca9554_get_pin(Object *obj, Visitor *v, const char *name,
         error_setg(errp, "%s: error reading %s", __func__, name);
         return;
     }
-    if (pin < 0 || pin > PCA9554_PIN_COUNT) {
+    if (pin < 0 || pin >= PCA9554_PIN_COUNT) {
         error_setg(errp, "%s invalid pin %s", __func__, name);
         return;
     }
@@ -187,7 +187,7 @@ static void pca9554_set_pin(Object *obj, Visitor *v, const char *name,
         error_setg(errp, "%s: error reading %s", __func__, name);
         return;
     }
-    if (pin < 0 || pin > PCA9554_PIN_COUNT) {
+    if (pin < 0 || pin >= PCA9554_PIN_COUNT) {
         error_setg(errp, "%s invalid pin %s", __func__, name);
         return;
     }
-- 
2.44.0

Re: [PATCH] misc/pca9554: Fix check of pin range value in property accessors

[Miles Glenn]

On Thu, 2024-03-21 at 17:01 +0100, Cédric Le Goater wrote:
> Coverity detected an "Integer handling" issue with the pin value :
> 
>   In expression "state >> pin", right shifting "state" by more than 7
>   bits always yields zero.  The shift amount, "pin", is as much as 8.
> 
> In practice, this should not happen because the properties "pin8" and
> above are not created. Nevertheless, fix the range to avoid this
> warning.
> 
> Fixes: CID 1534917
> Fixes: de0c7d543bca ("misc: Add a pca9554 GPIO device model")
> Cc: Glenn Miles <milesg@linux.vnet.ibm.com>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>  hw/misc/pca9554.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/misc/pca9554.c b/hw/misc/pca9554.c
> index
> 778b32e4430a8b618322c26b1b185ed3ead97cc4..5e31696797d9564666ece6fe177
> 37ee2a9733e96 100644
> --- a/hw/misc/pca9554.c
> +++ b/hw/misc/pca9554.c
> @@ -160,7 +160,7 @@ static void pca9554_get_pin(Object *obj, Visitor
> *v, const char *name,
>          error_setg(errp, "%s: error reading %s", __func__, name);
>          return;
>      }
> -    if (pin < 0 || pin > PCA9554_PIN_COUNT) {
> +    if (pin < 0 || pin >= PCA9554_PIN_COUNT) {
>          error_setg(errp, "%s invalid pin %s", __func__, name);
>          return;
>      }
> @@ -187,7 +187,7 @@ static void pca9554_set_pin(Object *obj, Visitor
> *v, const char *name,
>          error_setg(errp, "%s: error reading %s", __func__, name);
>          return;
>      }
> -    if (pin < 0 || pin > PCA9554_PIN_COUNT) {
> +    if (pin < 0 || pin >= PCA9554_PIN_COUNT) {
>          error_setg(errp, "%s invalid pin %s", __func__, name);
>          return;
>      }

Thanks, Cédric!  I guess I should be running coverity myself.

-Glenn

Reviewed-by: Glenn Miles <milesg@linux.vnet.ibm.com>

Re: [PATCH] misc/pca9554: Fix check of pin range value in property accessors

[Cédric Le Goater]

On 3/21/24 17:08, Miles Glenn wrote:
> On Thu, 2024-03-21 at 17:01 +0100, Cédric Le Goater wrote:
>> Coverity detected an "Integer handling" issue with the pin value :
>>
>>    In expression "state >> pin", right shifting "state" by more than 7
>>    bits always yields zero.  The shift amount, "pin", is as much as 8.
>>
>> In practice, this should not happen because the properties "pin8" and
>> above are not created. Nevertheless, fix the range to avoid this
>> warning.
>>
>> Fixes: CID 1534917
>> Fixes: de0c7d543bca ("misc: Add a pca9554 GPIO device model")
>> Cc: Glenn Miles <milesg@linux.vnet.ibm.com>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>   hw/misc/pca9554.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/misc/pca9554.c b/hw/misc/pca9554.c
>> index
>> 778b32e4430a8b618322c26b1b185ed3ead97cc4..5e31696797d9564666ece6fe177
>> 37ee2a9733e96 100644
>> --- a/hw/misc/pca9554.c
>> +++ b/hw/misc/pca9554.c
>> @@ -160,7 +160,7 @@ static void pca9554_get_pin(Object *obj, Visitor
>> *v, const char *name,
>>           error_setg(errp, "%s: error reading %s", __func__, name);
>>           return;
>>       }
>> -    if (pin < 0 || pin > PCA9554_PIN_COUNT) {
>> +    if (pin < 0 || pin >= PCA9554_PIN_COUNT) {
>>           error_setg(errp, "%s invalid pin %s", __func__, name);
>>           return;
>>       }
>> @@ -187,7 +187,7 @@ static void pca9554_set_pin(Object *obj, Visitor
>> *v, const char *name,
>>           error_setg(errp, "%s: error reading %s", __func__, name);
>>           return;
>>       }
>> -    if (pin < 0 || pin > PCA9554_PIN_COUNT) {
>> +    if (pin < 0 || pin >= PCA9554_PIN_COUNT) {
>>           error_setg(errp, "%s invalid pin %s", __func__, name);
>>           return;
>>       }
> 
> Thanks, Cédric!  I guess I should be running coverity myself.

I don't myself. I get reports from :

   https://scan.coverity.com/projects/qemu

Thanks,

C.

Re: [PATCH] misc/pca9554: Fix check of pin range value in property accessors

[Philippe Mathieu-Daudé]

On 21/3/24 17:01, Cédric Le Goater wrote:
> Coverity detected an "Integer handling" issue with the pin value :
> 
>    In expression "state >> pin", right shifting "state" by more than 7
>    bits always yields zero.  The shift amount, "pin", is as much as 8.
> 
> In practice, this should not happen because the properties "pin8" and
> above are not created. Nevertheless, fix the range to avoid this warning.
> 
> Fixes: CID 1534917
> Fixes: de0c7d543bca ("misc: Add a pca9554 GPIO device model")
> Cc: Glenn Miles <milesg@linux.vnet.ibm.com>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   hw/misc/pca9554.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Isn't it the one Peter fixed in
https://lore.kernel.org/qemu-devel/20240312183810.557768-5-peter.maydell@linaro.org/?

Re: [PATCH] misc/pca9554: Fix check of pin range value in property accessors

[Cédric Le Goater]

On 3/21/24 18:15, Philippe Mathieu-Daudé wrote:
> On 21/3/24 17:01, Cédric Le Goater wrote:
>> Coverity detected an "Integer handling" issue with the pin value :
>>
>>    In expression "state >> pin", right shifting "state" by more than 7
>>    bits always yields zero.  The shift amount, "pin", is as much as 8.
>>
>> In practice, this should not happen because the properties "pin8" and
>> above are not created. Nevertheless, fix the range to avoid this warning.
>>
>> Fixes: CID 1534917
>> Fixes: de0c7d543bca ("misc: Add a pca9554 GPIO device model")
>> Cc: Glenn Miles <milesg@linux.vnet.ibm.com>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>   hw/misc/pca9554.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Isn't it the one Peter fixed in
> https://lore.kernel.org/qemu-devel/20240312183810.557768-5-peter.maydell@linaro.org/?

Oh yes. I missed it. Hopefully, they are similar. Let's keep Peter's.

However, what I would like to do as a follow-up is to move the
hw/misc/pca955* models under hw/gpio/. Is it something we can do
for 9.0 ?

Thanks,

C.