| Message ID | 20191015083837.GA29104@mwanda (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | platform/x86: huawei-wmi: make validation stricter in huawei_wmi_battery_set() | expand |
On Tue, Oct 15, 2019 at 08:21:59AM -0400, Ayman Bagabas wrote: > Hi Dan > > On Tue, Oct 15, 2019, 4:39 AM Dan Carpenter <dan.carpenter@oracle.com> > wrote: > > > I don't think it makes sense for "end" to be negative or for even for it > > to be less than "start". That also means that "start" can't be more > > than 100 which is good. > > > > While this makes sense, you run into issues where you cannot set "start" > before "end" and vice versa. > > Take this scenario, you have start=70 and end=90, now you want to set these > to start=40 and end=60, you would have to set "start" first before you can > change the value of "end" otherwise you will run into EINVAL. Now imagine > you wanna go the opposite direction, you would have to set "end" before you > can change "start". > I think having a little wiggle room is fine for such scenarios. > I haven't tested this code... What you're describing sounds really very weird to me, but I will accept that you know more about your use cases than I do. My other concern is that right now you can set start > 100 or end < 0. regards, dan carpenter
On Tue, 2019-10-15 at 15:46 +0300, Dan Carpenter wrote: > On Tue, Oct 15, 2019 at 08:21:59AM -0400, Ayman Bagabas wrote: > > Hi Dan > > > > On Tue, Oct 15, 2019, 4:39 AM Dan Carpenter < > > dan.carpenter@oracle.com> > > wrote: > > > > > I don't think it makes sense for "end" to be negative or for even > > > for it > > > to be less than "start". That also means that "start" can't be > > > more > > > than 100 which is good. > > > > > > > While this makes sense, you run into issues where you cannot set > > "start" > > before "end" and vice versa. > > > > Take this scenario, you have start=70 and end=90, now you want to > > set these > > to start=40 and end=60, you would have to set "start" first before > > you can > > change the value of "end" otherwise you will run into EINVAL. Now > > imagine > > you wanna go the opposite direction, you would have to set "end" > > before you > > can change "start". > > I think having a little wiggle room is fine for such scenarios. > > > > I haven't tested this code... What you're describing sounds really > very weird to me, but I will accept that you know more about your > use cases than I do. > > My other concern is that right now you can set start > 100 or end < > 0. We should check for these cases. > > regards, > dan carpenter >
diff --git a/drivers/platform/x86/huawei-wmi.c b/drivers/platform/x86/huawei-wmi.c index 6720f78c60c2..02a505a72172 100644 --- a/drivers/platform/x86/huawei-wmi.c +++ b/drivers/platform/x86/huawei-wmi.c @@ -343,7 +343,7 @@ static int huawei_wmi_battery_set(int start, int end) union hwmi_arg arg; int err; - if (start < 0 || end > 100) + if (start < 0 || end < start || end > 100) return -EINVAL; arg.cmd = BATTERY_THRESH_SET;
I don't think it makes sense for "end" to be negative or for even for it to be less than "start". That also means that "start" can't be more than 100 which is good. Fixes: b7527d0f4502 ("platform/x86: huawei-wmi: Add battery charging thresholds") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- drivers/platform/x86/huawei-wmi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)