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
| ||
|
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
Powered by blists - more mailing lists