[PATCH] platform/x86: huawei-wmi: make validation stricter in huawei_wmi_battery_set()

[PATCH] platform/x86: huawei-wmi: make validation stricter in huawei_wmi_battery_set()

[Dan Carpenter]

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(-)

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;
-- 
2.20.1

[PATCH] platform/x86: huawei-wmi: make validation stricter in huawei_wmi_battery_set()

[Dan Carpenter]

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(-)

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;
-- 
2.20.1

Re: [PATCH] platform/x86: huawei-wmi: make validation stricter in huawei_wmi_battery_set()

[Dan Carpenter]

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

Re: [PATCH] platform/x86: huawei-wmi: make validation stricter in huawei_wmi_battery_set()

[Dan Carpenter]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="windows-1254", Size: 1098 bytes --]

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 startp and end, now you want to set these
> to start@ and end`, 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

Re: [PATCH] platform/x86: huawei-wmi: make validation stricter in huawei_wmi_battery_set()

[ayman.bagabas]

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
> 

Re: [PATCH] platform/x86: huawei-wmi: make validation stricter in huawei_wmi_battery_set()

[ayman.bagabas]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="windows-1254", Size: 1292 bytes --]

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 startp and end, now you want to
> > set these
> > to start@ and end`, 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
>