[PATCH wpan-tools] mac: range checking for command accepting only 0 or 1

[PATCH wpan-tools] mac: range checking for command accepting only 0 or 1

[Stefan Schmidt]

lbt and ackreq_default only accept 0or 1 as arguments which we did not
acount for so far. Testing invalid arguments and checking teh return
code uncovered this one.

Signed-off-by: Stefan Schmidt <stefan@osg.samsung.com>
---
 src/mac.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/mac.c b/src/mac.c
index 76db58f..26c6fc5 100644
--- a/src/mac.c
+++ b/src/mac.c
@@ -175,6 +175,8 @@ static int handle_lbt_mode(struct nl802154_state *state,
 	mode = strtoul(argv[0], &end, 0);
 	if (*end != '\0')
 		return 1;
+	if (mode != 0 && mode != 1)
+		return 1;
 
 	NLA_PUT_U8(msg, NL802154_ATTR_LBT_MODE, mode);
 
@@ -202,6 +204,8 @@ static int handle_ackreq_default(struct nl802154_state *state,
 	ackreq = strtoul(argv[0], &end, 0);
 	if (*end != '\0')
 		return 1;
+	if (ackreq != 0 && ackreq != 1)
+		return 1;
 
 	NLA_PUT_U8(msg, NL802154_ATTR_ACKREQ_DEFAULT, ackreq);
 
-- 
2.4.3

Re: [PATCH wpan-tools] mac: range checking for command accepting only 0 or 1

[Alexander Aring]

Hi Stefan,

On Wed, Aug 19, 2015 at 04:13:24PM +0200, Stefan Schmidt wrote:
> lbt and ackreq_default only accept 0or 1 as arguments which we did not
> acount for so far. Testing invalid arguments and checking teh return
> code uncovered this one.
> 
> Signed-off-by: Stefan Schmidt <stefan@osg.samsung.com>
> ---
>  src/mac.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/src/mac.c b/src/mac.c
> index 76db58f..26c6fc5 100644
> --- a/src/mac.c
> +++ b/src/mac.c
> @@ -175,6 +175,8 @@ static int handle_lbt_mode(struct nl802154_state *state,
>  	mode = strtoul(argv[0], &end, 0);
>  	if (*end != '\0')
>  		return 1;
> +	if (mode != 0 && mode != 1)
> +		return 1;
>  
>  	NLA_PUT_U8(msg, NL802154_ATTR_LBT_MODE, mode);
>  
> @@ -202,6 +204,8 @@ static int handle_ackreq_default(struct nl802154_state *state,
>  	ackreq = strtoul(argv[0], &end, 0);
>  	if (*end != '\0')
>  		return 1;
> +	if (ackreq != 0 && ackreq != 1)
> +		return 1;
>  

range checks should be handled by kernelspace. Currently we do a "!!var"
conversion there. Maybe we should change it there and accept "1" or "0".

We should never handle "if a range is valid or not" inside userspace,
otherwise other applications which talking to nl802154 can do a
different handling then.

- Alex

Re: [PATCH wpan-tools] mac: range checking for command accepting only 0 or 1

[Stefan Schmidt]

Hello.

On 19/08/15 16:22, Alexander Aring wrote:
> Hi Stefan,
>
> On Wed, Aug 19, 2015 at 04:13:24PM +0200, Stefan Schmidt wrote:
>> lbt and ackreq_default only accept 0or 1 as arguments which we did not
>> acount for so far. Testing invalid arguments and checking teh return
>> code uncovered this one.
>>
>> Signed-off-by: Stefan Schmidt <stefan@osg.samsung.com>
>> ---
>>   src/mac.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/src/mac.c b/src/mac.c
>> index 76db58f..26c6fc5 100644
>> --- a/src/mac.c
>> +++ b/src/mac.c
>> @@ -175,6 +175,8 @@ static int handle_lbt_mode(struct nl802154_state *state,
>>   	mode = strtoul(argv[0], &end, 0);
>>   	if (*end != '\0')
>>   		return 1;
>> +	if (mode != 0 && mode != 1)
>> +		return 1;
>>   
>>   	NLA_PUT_U8(msg, NL802154_ATTR_LBT_MODE, mode);
>>   
>> @@ -202,6 +204,8 @@ static int handle_ackreq_default(struct nl802154_state *state,
>>   	ackreq = strtoul(argv[0], &end, 0);
>>   	if (*end != '\0')
>>   		return 1;
>> +	if (ackreq != 0 && ackreq != 1)
>> +		return 1;
>>   
> range checks should be handled by kernelspace. Currently we do a "!!var"
> conversion there.

Which is why any value > 0 gets set to 1 here.

> Maybe we should change it there and accept "1" or "0".

I think that would indeed be better.

> We should never handle "if a range is valid or not" inside userspace,
> otherwise other applications which talking to nl802154 can do a
> different handling then.

I disagree here. We should for sure do range checking in the kernel but 
we should as well check for it in iwpan and get the return code right.

regards
Stefan Schmidt

Re: [PATCH wpan-tools] mac: range checking for command accepting only 0 or 1

[Alexander Aring]

On Wed, Aug 19, 2015 at 04:28:33PM +0200, Stefan Schmidt wrote:
> Hello.
> 
> On 19/08/15 16:22, Alexander Aring wrote:
> >Hi Stefan,
> >
> >On Wed, Aug 19, 2015 at 04:13:24PM +0200, Stefan Schmidt wrote:
> >>lbt and ackreq_default only accept 0or 1 as arguments which we did not
> >>acount for so far. Testing invalid arguments and checking teh return
> >>code uncovered this one.
> >>
> >>Signed-off-by: Stefan Schmidt <stefan@osg.samsung.com>
> >>---
> >>  src/mac.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >>diff --git a/src/mac.c b/src/mac.c
> >>index 76db58f..26c6fc5 100644
> >>--- a/src/mac.c
> >>+++ b/src/mac.c
> >>@@ -175,6 +175,8 @@ static int handle_lbt_mode(struct nl802154_state *state,
> >>  	mode = strtoul(argv[0], &end, 0);
> >>  	if (*end != '\0')
> >>  		return 1;
> >>+	if (mode != 0 && mode != 1)
> >>+		return 1;
> >>  	NLA_PUT_U8(msg, NL802154_ATTR_LBT_MODE, mode);
> >>@@ -202,6 +204,8 @@ static int handle_ackreq_default(struct nl802154_state *state,
> >>  	ackreq = strtoul(argv[0], &end, 0);
> >>  	if (*end != '\0')
> >>  		return 1;
> >>+	if (ackreq != 0 && ackreq != 1)
> >>+		return 1;
> >range checks should be handled by kernelspace. Currently we do a "!!var"
> >conversion there.
> 
> Which is why any value > 0 gets set to 1 here.
> 
> >Maybe we should change it there and accept "1" or "0".
> 
> I think that would indeed be better.
> 
> >We should never handle "if a range is valid or not" inside userspace,
> >otherwise other applications which talking to nl802154 can do a
> >different handling then.
> 
> I disagree here. We should for sure do range checking in the kernel but we
> should as well check for it in iwpan and get the return code right.
> 

this should be handled by returning -EINVAL in the handler of nl802154. Try a:

iwpan dev wpan0 set max_frame_retries 8

which should always return "-EINVAL" inside nl802154.

The return is:

command failed: Invalid argument (-22)



Is this okay? Or do you expect a different handling?

- Alex

Re: [PATCH wpan-tools] mac: range checking for command accepting only 0 or 1

[Alexander Aring]

On Wed, Aug 19, 2015 at 04:33:31PM +0200, Alexander Aring wrote:
> On Wed, Aug 19, 2015 at 04:28:33PM +0200, Stefan Schmidt wrote:
> > Hello.
> > 
> > On 19/08/15 16:22, Alexander Aring wrote:
> > >Hi Stefan,
> > >
> > >On Wed, Aug 19, 2015 at 04:13:24PM +0200, Stefan Schmidt wrote:
> > >>lbt and ackreq_default only accept 0or 1 as arguments which we did not
> > >>acount for so far. Testing invalid arguments and checking teh return
> > >>code uncovered this one.
> > >>
> > >>Signed-off-by: Stefan Schmidt <stefan@osg.samsung.com>
> > >>---
> > >>  src/mac.c | 4 ++++
> > >>  1 file changed, 4 insertions(+)
> > >>
> > >>diff --git a/src/mac.c b/src/mac.c
> > >>index 76db58f..26c6fc5 100644
> > >>--- a/src/mac.c
> > >>+++ b/src/mac.c
> > >>@@ -175,6 +175,8 @@ static int handle_lbt_mode(struct nl802154_state *state,
> > >>  	mode = strtoul(argv[0], &end, 0);
> > >>  	if (*end != '\0')
> > >>  		return 1;
> > >>+	if (mode != 0 && mode != 1)
> > >>+		return 1;
> > >>  	NLA_PUT_U8(msg, NL802154_ATTR_LBT_MODE, mode);
> > >>@@ -202,6 +204,8 @@ static int handle_ackreq_default(struct nl802154_state *state,
> > >>  	ackreq = strtoul(argv[0], &end, 0);
> > >>  	if (*end != '\0')
> > >>  		return 1;
> > >>+	if (ackreq != 0 && ackreq != 1)
> > >>+		return 1;
> > >range checks should be handled by kernelspace. Currently we do a "!!var"
> > >conversion there.
> > 
> > Which is why any value > 0 gets set to 1 here.
> > 
> > >Maybe we should change it there and accept "1" or "0".
> > 
> > I think that would indeed be better.
> > 
> > >We should never handle "if a range is valid or not" inside userspace,
> > >otherwise other applications which talking to nl802154 can do a
> > >different handling then.
> > 
> > I disagree here. We should for sure do range checking in the kernel but we
> > should as well check for it in iwpan and get the return code right.
> > 
> 
> this should be handled by returning -EINVAL in the handler of nl802154. Try a:
> 
> iwpan dev wpan0 set max_frame_retries 8
> 
> which should always return "-EINVAL" inside nl802154.
> 
> The return is:
> 
> command failed: Invalid argument (-22)
> 
> 
> 
> Is this okay? Or do you expect a different handling?
> 

I know you can't see this handling inside the command handler, this is
handled somehow by iwpan.c and netlink magic. :-)

- Alex

Re: [PATCH wpan-tools] mac: range checking for command accepting only 0 or 1

[Stefan Schmidt]

Hello.

On 19/08/15 16:33, Alexander Aring wrote:
> On Wed, Aug 19, 2015 at 04:28:33PM +0200, Stefan Schmidt wrote:
>> Hello.
>>
>> On 19/08/15 16:22, Alexander Aring wrote:
>>> Hi Stefan,
>>>
>>> On Wed, Aug 19, 2015 at 04:13:24PM +0200, Stefan Schmidt wrote:
>>>> lbt and ackreq_default only accept 0or 1 as arguments which we did not
>>>> acount for so far. Testing invalid arguments and checking teh return
>>>> code uncovered this one.
>>>>
>>>> Signed-off-by: Stefan Schmidt <stefan@osg.samsung.com>
>>>> ---
>>>>   src/mac.c | 4 ++++
>>>>   1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/src/mac.c b/src/mac.c
>>>> index 76db58f..26c6fc5 100644
>>>> --- a/src/mac.c
>>>> +++ b/src/mac.c
>>>> @@ -175,6 +175,8 @@ static int handle_lbt_mode(struct nl802154_state *state,
>>>>   	mode = strtoul(argv[0], &end, 0);
>>>>   	if (*end != '\0')
>>>>   		return 1;
>>>> +	if (mode != 0 && mode != 1)
>>>> +		return 1;
>>>>   	NLA_PUT_U8(msg, NL802154_ATTR_LBT_MODE, mode);
>>>> @@ -202,6 +204,8 @@ static int handle_ackreq_default(struct nl802154_state *state,
>>>>   	ackreq = strtoul(argv[0], &end, 0);
>>>>   	if (*end != '\0')
>>>>   		return 1;
>>>> +	if (ackreq != 0 && ackreq != 1)
>>>> +		return 1;
>>> range checks should be handled by kernelspace. Currently we do a "!!var"
>>> conversion there.
>> Which is why any value > 0 gets set to 1 here.
>>
>>> Maybe we should change it there and accept "1" or "0".
>> I think that would indeed be better.
>>
>>> We should never handle "if a range is valid or not" inside userspace,
>>> otherwise other applications which talking to nl802154 can do a
>>> different handling then.
>> I disagree here. We should for sure do range checking in the kernel but we
>> should as well check for it in iwpan and get the return code right.
>>
> this should be handled by returning -EINVAL in the handler of nl802154. Try a:
>
> iwpan dev wpan0 set max_frame_retries 8
>
> which should always return "-EINVAL" inside nl802154.
>
> The return is:
>
> command failed: Invalid argument (-22)
>
>
>
> Is this okay? Or do you expect a different handling?
>
The problem is with lbt and and autoreq_default which are boolean. We 
accept things like 400 just fine here and bring it down to 1.
I'm going to fix this in nl802154 and leave it out of wpan-tools.

regards
Stefan Schmidt

Re: [PATCH wpan-tools] mac: range checking for command accepting only 0 or 1

[Alexander Aring]

On Wed, Aug 19, 2015 at 05:27:01PM +0200, Stefan Schmidt wrote:
> The problem is with lbt and and autoreq_default which are boolean. We accept
> things like 400 just fine here and bring it down to 1.
> I'm going to fix this in nl802154 and leave it out of wpan-tools.
> 

We finally has the result that the userspace need to convert the value
then which is "unsigned long" and the netlink command requires "U8" or
"U16".

Then the command need check the range when it's U8:

0x00 - 0xff

and when it's U16:

0x0000 - 0xffff

The rest of ranges checks will do the kernel then. This will remove the
overflow in userspace e.g.

iwpan dev wpan0 set ackreq_default 256

will set the ackreq_default to 0. This check can't be done at
kernelspace side. The kernel need to check then if it's "0" or "1" only.

We need to introduce this behabiour for all U8 and U16 settings then.

- Alex