Return-Path: From: Szymon Janc To: Bastien Nocera Cc: linux-bluetooth@vger.kernel.org Subject: Re: [PATCH 3/9] sixaxis: Ask user whether cable configuration should be allowed Date: Wed, 27 Sep 2017 11:14:24 +0200 Message-ID: <5909706.3V1B56IrXH@ix> In-Reply-To: <20170904181212.5639-3-hadess@hadess.net> References: <20170904181212.5639-1-hadess@hadess.net> <20170904181212.5639-3-hadess@hadess.net> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Bastien, On Monday, 4 September 2017 20:12:06 CEST Bastien Nocera wrote: > Previously, users doing cable configuration of Sixaxis PS3 controllers > would only get asked whether a device was allowed to connect to the > computer when switching it to Bluetooth mode: unplugging it, and > pressing the PS button. > > Instead, we should ask the user straight away, through the agent, > whether the pad should be allowed to connect. > > This makes it easier to setup those devices, while keeping security. > --- > plugins/sixaxis.c | 83 > +++++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 63 > insertions(+), 20 deletions(-) > > diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c > index 7d3a8f3ac..c8305d52f 100644 > --- a/plugins/sixaxis.c > +++ b/plugins/sixaxis.c > @@ -44,6 +44,7 @@ > > #include "src/adapter.h" > #include "src/device.h" > +#include "src/agent.h" > #include "src/plugin.h" > #include "src/log.h" > #include "src/shared/util.h" > @@ -71,6 +72,13 @@ static const struct { > }, > }; > > +struct authentication_closure { > + struct btd_adapter *adapter; > + struct btd_device *device; > + int fd; > + char device_addr[18]; > +}; > + > static struct udev *ctx = NULL; > static struct udev_monitor *monitor = NULL; > static guint watch_id = 0; > @@ -135,19 +143,51 @@ static int set_master_bdaddr(int fd, const bdaddr_t > *bdaddr) return ret; > } > > +static void agent_auth_cb(DBusError *derr, > + void *user_data) > +{ > + struct authentication_closure *closure = user_data; > + char master_addr[18], adapter_addr[18]; > + bdaddr_t master_bdaddr; > + const bdaddr_t *adapter_bdaddr; > + > + if (derr != NULL) { > + DBG("Agent replied negatively, removing temporary device"); > + goto error; > + } > + > + btd_device_set_temporary(closure->device, false); > + > + if (get_master_bdaddr(closure->fd, &master_bdaddr) < 0) > + goto error; > + > + adapter_bdaddr = btd_adapter_get_address(closure->adapter); > + if (bacmp(adapter_bdaddr, &master_bdaddr)) { > + if (set_master_bdaddr(closure->fd, adapter_bdaddr) < 0) > + goto error; > + } > + > + ba2str(&master_bdaddr, master_addr); > + ba2str(adapter_bdaddr, adapter_addr); > + DBG("remote %s old_master %s new_master %s", > + closure->device_addr, master_addr, adapter_addr); > + > +error: > + close(closure->fd); > + g_free(closure); > +} > + > static bool setup_device(int fd, int index, struct btd_adapter *adapter) > { > - char device_addr[18], master_addr[18], adapter_addr[18]; > - bdaddr_t device_bdaddr, master_bdaddr; > + char device_addr[18]; > + bdaddr_t device_bdaddr; > const bdaddr_t *adapter_bdaddr; > struct btd_device *device; > + struct authentication_closure *closure; > > if (get_device_bdaddr(fd, &device_bdaddr) < 0) > return false; > > - if (get_master_bdaddr(fd, &master_bdaddr) < 0) > - return false; > - > /* This can happen if controller was plugged while already connected > * eg. to charge up battery. */ > device = btd_adapter_find_device(adapter, &device_bdaddr, > @@ -155,25 +195,14 @@ static bool setup_device(int fd, int index, struct > btd_adapter *adapter) if (device && btd_device_is_connected(device)) > return false; > > - adapter_bdaddr = btd_adapter_get_address(adapter); > - > - if (bacmp(adapter_bdaddr, &master_bdaddr)) { > - if (set_master_bdaddr(fd, adapter_bdaddr) < 0) > - return false; > - } > - > ba2str(&device_bdaddr, device_addr); > - ba2str(&master_bdaddr, master_addr); > - ba2str(adapter_bdaddr, adapter_addr); > - DBG("remote %s old_master %s new_master %s", > - device_addr, master_addr, adapter_addr); > > device = btd_adapter_get_device(adapter, &device_bdaddr, BDADDR_BREDR); > > if (g_slist_find_custom(btd_device_get_uuids(device), HID_UUID, > (GCompareFunc)strcasecmp)) { > DBG("device %s already known, skipping", device_addr); > - return true; > + return false; > } > > info("sixaxis: setting up new device"); > @@ -181,7 +210,20 @@ static bool setup_device(int fd, int index, struct > btd_adapter *adapter) btd_device_device_set_name(device, > devices[index].name); > btd_device_set_pnpid(device, devices[index].source, devices[index].vid, > devices[index].pid, devices[index].version); > - btd_device_set_temporary(device, false); > + btd_device_set_temporary(device, true); > + > + closure = g_try_new0(struct authentication_closure, 1); > + if (!closure) { > + btd_adapter_remove_device(adapter, device); > + return false; > + } > + closure->adapter = adapter; > + closure->device = device; > + closure->fd = fd; > + memcpy(&closure->device_addr, device_addr, sizeof(device_addr)); > + adapter_bdaddr = btd_adapter_get_address(adapter); > + btd_request_authorization_cable_configured(adapter_bdaddr, &device_bdaddr, > + HID_UUID, agent_auth_cb, closure); > > return true; > } > @@ -236,8 +278,9 @@ static void device_added(struct udev_device *udevice) > if (fd < 0) > return; > > - setup_device(fd, index, adapter); > - close(fd); > + /* Only close the fd if an authentication is not pending */ > + if (!setup_device(fd, index, adapter)) > + close(fd); > } > > static gboolean monitor_watch(GIOChannel *source, GIOCondition condition, So user will be asked for consent after DS3 was configured? Not that I have strong opinion on this but did you consider configuring it only after user gave consent? -- pozdrawiam Szymon Janc