On Mon, Mar 16, 2020 at 09:29:43AM +0100, Jasper Lievisse Adriaanse wrote: > Hi, > > The type of brightness and video_output is uint32_t; therefore it > can never be less than 0 (which is what HCI_LCD_BRIGHTNESS_MIN and > HCI_VIDEO_OUTPUT_CYCLE_MIN are defined to). So trim the checks by > removig the impossible cases. > > Coverity CID 1453109, 1453169 > > OK?
These values ultimately come from aml_val2int() which returns int64_t, i.e. a signed value. This driver currently copies that value to a uint32_t, shifts it, and this shifted value ends up being copied to an int, and then again to a uint32_t, which is then range-checked. I don't understand why a range check is done only that late in the game, after multiple integer type conversions that seem to be performed for no good reason other than that the expected range will fit into an int. So an alternative fix could be to switch brightness values in this driver to int64_t everywhere, i.e. in toshiba_get_brightness(), toshiba_find_brightness(), toshiba_set_brightness(), etc. Then you can keep all the code as it is. Or read an int64_t with aml_val2int(), shift and range-check it right away, and only convert to a narrower type if the value is in the expected range. Else error. > Index: acpi/acpitoshiba.c > =================================================================== > RCS file: /cvs/src/sys/dev/acpi/acpitoshiba.c,v > retrieving revision 1.12 > diff -u -p -r1.12 acpitoshiba.c > --- acpi/acpitoshiba.c 13 Oct 2019 10:56:31 -0000 1.12 > +++ acpi/acpitoshiba.c 11 Mar 2020 11:35:23 -0000 > @@ -438,9 +438,8 @@ toshiba_set_brightness(struct acpitoshib > for (i = 0; i < HCI_WORDS; ++i) > args[i].type = AML_OBJTYPE_INTEGER; > > - if ((*brightness < HCI_LCD_BRIGHTNESS_MIN) || > - (*brightness > HCI_LCD_BRIGHTNESS_MAX)) > - return (HCI_FAILURE); > + if (*brightness > HCI_LCD_BRIGHTNESS_MAX) > + return (HCI_FAILURE); > > *brightness <<= HCI_LCD_BRIGHTNESS_SHIFT; > > @@ -534,8 +533,7 @@ toshiba_set_video_output(struct acpitosh > > bzero(args, sizeof(args)); > > - if ((*video_output < HCI_VIDEO_OUTPUT_CYCLE_MIN) || > - (*video_output > HCI_VIDEO_OUTPUT_CYCLE_MAX)) > + if (*video_output > HCI_VIDEO_OUTPUT_CYCLE_MAX) > return (HCI_FAILURE); > > *video_output |= HCI_VIDEO_OUTPUT_FLAG; > -- > jasper > >
