linux-kernel - Re: [PATCH 8/9] rfkill: Userspace control for airplane mode

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]

Message-ID: <CA+A7VXU9zoQZ5wM2Ranf7o9vUphGb0THkh8-TUM_1yD7aC30pQ@mail.gmail.com>
Date:	Tue, 9 Feb 2016 16:35:19 -0500
From:	João Paulo Rechi Vita <jprvita@...il.com>
To:	Julian Calaby <julian.calaby@...il.com>
Cc:	Johannes Berg <johannes@...solutions.net>,
	"David S. Miller" <davem@...emloft.net>,
	Darren Hart <dvhart@...radead.org>,
	linux-wireless <linux-wireless@...r.kernel.org>,
	netdev <netdev@...r.kernel.org>,
	platform-driver-x86@...r.kernel.org, linux-api@...r.kernel.org,
	linux-doc@...r.kernel.org,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	linux@...lessm.com,
	João Paulo Rechi Vita <jprvita@...lessm.com>
Subject: Re: [PATCH 8/9] rfkill: Userspace control for airplane mode

On 8 February 2016 at 17:53, Julian Calaby <julian.calaby@...il.com> wrote:

>> +       if (ev.op == RFKILL_OP_AIRPLANE_MODE_RELEASE) {
>> +               if (rfkill_apm_owned && !data->is_apm_owner) {
>
> Are you sure this is correct?
>
> In the case that airplane mode isn't owned, the
> rfkill_apm_led_trigger_event() call will be a noop, so we should
> arguably not be calling it.
>

Ok, I'm changing the check to be consistent with _CHANGE, so the call
only succeeds if (rfkill_apm_owned && data->is_apm_owner), and return
an error otherwise.

> Also, should we just fail silently if we're not the owner? I.e. what
> does userspace learn from this op failing and is that useful?
>

I think it is better to return an error every time userspace is trying
to call an operation that it was not supposed to call at a certain
state (without acquiring control of the airplane-mode indicator). If a
program has a logic error that makes it call _RELEASE without calling
_ACQUIRE first, it's easier for the programmer to spot the problem if
we return an error here.

>> +                       count = -EACCES;
>> +               } else {
>> +                       bool state = rfkill_global_states[RFKILL_TYPE_ALL].cur;
>> +
>> +                       rfkill_apm_owned = false;
>> +                       data->is_apm_owner = false;
>> +                       rfkill_apm_led_trigger_event(state);
>> +               }
>> +       }
>> +
>> +       if (ev.op == RFKILL_OP_AIRPLANE_MODE_CHANGE) {
>> +               if (rfkill_apm_owned && data->is_apm_owner)
>> +                       rfkill_apm_led_trigger_event(ev.soft);
>> +               else
>> +                       count = -EACCES;
>> +       }
>> +
>>         if (ev.op == RFKILL_OP_CHANGE_ALL)
>>                 rfkill_update_global_state(ev.type, ev.soft);
>>
>> @@ -1230,7 +1261,17 @@ static int rfkill_fop_release(struct inode *inode, struct file *file)
>>         struct rfkill_int_event *ev, *tmp;
>>
>>         mutex_lock(&rfkill_global_mutex);
>> +
>> +       if (data->is_apm_owner) {
>> +               bool state = rfkill_global_states[RFKILL_TYPE_ALL].cur;
>> +
>> +               rfkill_apm_owned = false;
>> +               data->is_apm_owner = false;
>> +               rfkill_apm_led_trigger_event(state);
>
> Also, this code is duplicated from the _RELEASE op above. Would it
> make sense to factor it out into a separate function?
>

Yes, makes sense. This also made me notice I was assigning a negative
value to a size_t variable (count).

>> +       }
>> +
>>         list_del(&data->list);
>> +
>
> (extra line)
>

After factoring out the _RELEASE code it looks better without this
additional line.

>>         mutex_unlock(&rfkill_global_mutex);
>>
>>         mutex_destroy(&data->mtx);
>
> Thanks,
>

Thanks for the review, Julian. I'm sending an updated version.

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