linux-kernel - Re: [PATCHv2 08/10] rfkill: Use switch to demux userspace operations

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 29 Feb 2016 17:30:20 -0500
From:	João Paulo Rechi Vita <jprvita@...il.com>
To:	Jouni Malinen <j@...fi>
Cc:	Johannes Berg <johannes@...solutions.net>,
	"David S. Miller" <davem@...emloft.net>,
	Darren Hart <dvhart@...radead.org>,
	linux-wireless <linux-wireless@...r.kernel.org>,
	Network Development <netdev@...r.kernel.org>,
	platform-driver-x86@...r.kernel.org, linux-api@...r.kernel.org,
	linux-doc@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
	linux@...lessm.com,
	João Paulo Rechi Vita <jprvita@...lessm.com>
Subject: Re: [PATCHv2 08/10] rfkill: Use switch to demux userspace operations

Hello Jouni,

On 26 February 2016 at 12:59, Jouni Malinen <j@...fi> wrote:
> On Mon, Feb 22, 2016 at 11:36:39AM -0500, João Paulo Rechi Vita wrote:
>> Using a switch to handle different ev.op values in rfkill_fop_write()
>> makes the code easier to extend, as out-of-range values can always be
>> handled by the default case.
>
> This breaks rfkill.. There are automated test scripts for testing this
> area (and most of Wi-Fi for that matter. It would be nice if these were
> used for changes before they get contributed upstream..
>
> http://buildbot.w1.fi/hwsim/
>

Thanks for pointing that out, I haven't heard of this tool before.
I'll give it a try before my next submission.

> This specific commit broke all the rfkill_* test cases because of
> following:
>
>> diff --git a/net/rfkill/core.c b/net/rfkill/core.c
>> @@ -1199,29 +1200,32 @@ static ssize_t rfkill_fop_write(struct file *file, const char __user *buf,
>> -     list_for_each_entry(rfkill, &rfkill_list, node) {
>> -             if (rfkill->idx != ev.idx && ev.op != RFKILL_OP_CHANGE_ALL)
>> -                     continue;
>> -
>> -             if (rfkill->type != ev.type && ev.type != RFKILL_TYPE_ALL)
>> -                     continue;
>
> Note that RFKILL_TYPE_ALL here..
>
>> +             list_for_each_entry(rfkill, &rfkill_list, node)
>> +                     if (rfkill->type == ev.type ||
>> +                         ev.type == RFKILL_TYPE_ALL)
>> +                             rfkill_set_block(rfkill, ev.soft);
>
> It was included for RFKILL_OP_CHANGE_ALL.
>
>> +     case RFKILL_OP_CHANGE:
>> +             list_for_each_entry(rfkill, &rfkill_list, node)
>> +                     if (rfkill->idx == ev.idx && rfkill->type == ev.type)
>> +                             rfkill_set_block(rfkill, ev.soft);
>
> but not for RFKILL_OP_CHANGE..
>
> This needs following to work:
>
>
> diff --git a/net/rfkill/core.c b/net/rfkill/core.c
> index 59ff92d..c4bbd19 100644
> --- a/net/rfkill/core.c
> +++ b/net/rfkill/core.c
> @@ -1239,7 +1239,9 @@ static ssize_t rfkill_fop_write(struct file *file, const char __user *buf,
>                 break;
>         case RFKILL_OP_CHANGE:
>                 list_for_each_entry(rfkill, &rfkill_list, node)
> -                       if (rfkill->idx == ev.idx && rfkill->type == ev.type)
> +                       if (rfkill->idx == ev.idx &&
> +                           (rfkill->type == ev.type ||
> +                            ev.type == RFKILL_TYPE_ALL))
>                                 rfkill_set_block(rfkill, ev.soft);
>                 ret = 0;
>                 break;
>

I agree there is a difference in the logic here, thanks for taking the
time to point it out so clearly, and sorry for missing this. But AFAIU
userspace should not call RFKILL_OP_CHANGE with ev.type ==
RFKILL_TYPE_ALL, as RFKILL_OP_CHANGE is intended to be used to
block/unblock one RFKill switch, and it is not possible to create a
RFKill switch with type == RFKILL_TYPE_ALL (rfkill_alloc() would
return NULL).

I tried to look into the source code of the test suite you pointed,
but couldn't easily figure out how it ends up with that combination.
Could you please explain (or point me in the code) how is that a valid
operation? If I'm not missing anything, we should probably return
EINVAL in this case.

Regards,

--
João Paulo Rechi Vita
http://about.me/jprvita