| Message ID | 20240321160154.901829-1-clg@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | misc/pca9554: Fix check of pin range value in property accessors | expand |
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>
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.
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/?
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.
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; }
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(-)