| Message ID | 20200406165542.v1.1.Ibfc500cbf0bf2dc8429b17f064e960e95bb228e9@changeid (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Marcel Holtmann |
| Headers | show |
| Series | [v1] Bluetooth: debugfs option to unset MITM flag | expand |
Hi Archie, > The BT qualification test SM/MAS/PKE/BV-01-C needs us to turn off > the MITM flag when pairing, and at the same time also set the io > capability to something other than no input no output. > > Currently the MITM flag is only unset when the io capability is set > to no input no output, therefore the test cannot be executed. > > This patch introduces a debugfs option for controlling whether MITM > flag should be set based on io capability. > > Signed-off-by: Archie Pusaka <apusaka@chromium.org> > --- > > include/net/bluetooth/hci.h | 1 + > net/bluetooth/smp.c | 52 ++++++++++++++++++++++++++++++++++++- > 2 files changed, 52 insertions(+), 1 deletion(-) > > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h > index 79de2a659dd69..5e183487c7479 100644 > --- a/include/net/bluetooth/hci.h > +++ b/include/net/bluetooth/hci.h > @@ -298,6 +298,7 @@ enum { > HCI_FORCE_STATIC_ADDR, > HCI_LL_RPA_RESOLUTION, > HCI_CMD_PENDING, > + HCI_ENFORCE_MITM_SMP, actually don’t you mean HCI_FORCE_NO_MITM? From your description, you want a toggle that disables MITM no matter what. > __HCI_NUM_FLAGS, > }; > diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c > index d0b695ee49f63..4fa8b112fb607 100644 > --- a/net/bluetooth/smp.c > +++ b/net/bluetooth/smp.c > @@ -2396,7 +2396,8 @@ int smp_conn_security(struct hci_conn *hcon, __u8 sec_level) > /* Require MITM if IO Capability allows or the security level > * requires it. > */ > - if (hcon->io_capability != HCI_IO_NO_INPUT_OUTPUT || > + if ((hci_dev_test_flag(hcon->hdev, HCI_ENFORCE_MITM_SMP) && > + hcon->io_capability != HCI_IO_NO_INPUT_OUTPUT) || > hcon->pending_sec_level > BT_SECURITY_MEDIUM) > authreq |= SMP_AUTH_MITM; /* New comment for this case .. if (!hci_dev_test_flag(hcon->hdev, HCI_FORCE_NO_MITM)) { /* Move comment here .. if (hcon->io_capability != HCI_IO_NO_INPUT_OUTPUT || hcon->pending_sec_level > BT_SECURITY_MEDIUM) authreq |= SMP_AUTH_MITM; } > > @@ -3402,6 +3403,50 @@ static const struct file_operations force_bredr_smp_fops = { > .llseek = default_llseek, > }; > > +static ssize_t enforce_mitm_smp_read(struct file *file, > + char __user *user_buf, > + size_t count, loff_t *ppos) > +{ > + struct hci_dev *hdev = file->private_data; > + char buf[3]; > + > + buf[0] = hci_dev_test_flag(hdev, HCI_ENFORCE_MITM_SMP) ? 'Y' : 'N'; > + buf[1] = '\n'; > + buf[2] = '\0'; > + return simple_read_from_buffer(user_buf, count, ppos, buf, 2); > +} > + > +static ssize_t enforce_mitm_smp_write(struct file *file, > + const char __user *user_buf, > + size_t count, loff_t *ppos) > +{ > + struct hci_dev *hdev = file->private_data; > + char buf[32]; > + size_t buf_size = min(count, (sizeof(buf) - 1)); > + bool enable; > + > + if (copy_from_user(buf, user_buf, buf_size)) > + return -EFAULT; > + > + buf[buf_size] = '\0'; > + if (strtobool(buf, &enable)) > + return -EINVAL; > + > + if (enable == hci_dev_test_flag(hdev, HCI_ENFORCE_MITM_SMP)) > + return -EALREADY; > + > + hci_dev_change_flag(hdev, HCI_ENFORCE_MITM_SMP); > + > + return count; > +} > + > +static const struct file_operations enforce_mitm_smp_fops = { > + .open = simple_open, > + .read = enforce_mitm_smp_read, > + .write = enforce_mitm_smp_write, > + .llseek = default_llseek, > +}; > + > int smp_register(struct hci_dev *hdev) > { > struct l2cap_chan *chan; > @@ -3426,6 +3471,11 @@ int smp_register(struct hci_dev *hdev) > > hdev->smp_data = chan; > > + /* Enforce the policy of determining MITM flag by io capabilities. */ > + hci_dev_set_flag(hdev, HCI_ENFORCE_MITM_SMP); No. Lets keep the current behavior the default. > + debugfs_create_file("enforce_mitm_smp", 0644, hdev->debugfs, hdev, > + &enforce_mitm_smp_fops); > + And this needs to move into hci_debugfs.c. Regards Marcel
Hi Marcel, The way I implemented it is, if HCI_ENFORCE_MITM_SMP is set (which it is by default), then it will assume the default behavior. However, if it is toggled to false, then it will not set the MITM flag although the io capability supports that. I am reluctant to use names with "no" on it, especially since it is a boolean. But if it is OK then I shall update to HCI_FORCE_NO_MITM, this way it will become more separable with the default behavior. Sure, I will move that to hci_debugfs.c. Thanks, Archie On Mon, 6 Apr 2020 at 19:46, Marcel Holtmann <marcel@holtmann.org> wrote: > > Hi Archie, > > > The BT qualification test SM/MAS/PKE/BV-01-C needs us to turn off > > the MITM flag when pairing, and at the same time also set the io > > capability to something other than no input no output. > > > > Currently the MITM flag is only unset when the io capability is set > > to no input no output, therefore the test cannot be executed. > > > > This patch introduces a debugfs option for controlling whether MITM > > flag should be set based on io capability. > > > > Signed-off-by: Archie Pusaka <apusaka@chromium.org> > > --- > > > > include/net/bluetooth/hci.h | 1 + > > net/bluetooth/smp.c | 52 ++++++++++++++++++++++++++++++++++++- > > 2 files changed, 52 insertions(+), 1 deletion(-) > > > > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h > > index 79de2a659dd69..5e183487c7479 100644 > > --- a/include/net/bluetooth/hci.h > > +++ b/include/net/bluetooth/hci.h > > @@ -298,6 +298,7 @@ enum { > > HCI_FORCE_STATIC_ADDR, > > HCI_LL_RPA_RESOLUTION, > > HCI_CMD_PENDING, > > + HCI_ENFORCE_MITM_SMP, > > actually don’t you mean HCI_FORCE_NO_MITM? From your description, you want a toggle that disables MITM no matter what. > > > __HCI_NUM_FLAGS, > > }; > > diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c > > index d0b695ee49f63..4fa8b112fb607 100644 > > --- a/net/bluetooth/smp.c > > +++ b/net/bluetooth/smp.c > > @@ -2396,7 +2396,8 @@ int smp_conn_security(struct hci_conn *hcon, __u8 sec_level) > > /* Require MITM if IO Capability allows or the security level > > * requires it. > > */ > > - if (hcon->io_capability != HCI_IO_NO_INPUT_OUTPUT || > > + if ((hci_dev_test_flag(hcon->hdev, HCI_ENFORCE_MITM_SMP) && > > + hcon->io_capability != HCI_IO_NO_INPUT_OUTPUT) || > > hcon->pending_sec_level > BT_SECURITY_MEDIUM) > > authreq |= SMP_AUTH_MITM; > > /* New comment for this case .. > if (!hci_dev_test_flag(hcon->hdev, HCI_FORCE_NO_MITM)) { > /* Move comment here .. > if (hcon->io_capability != HCI_IO_NO_INPUT_OUTPUT || > hcon->pending_sec_level > BT_SECURITY_MEDIUM) > authreq |= SMP_AUTH_MITM; > } > > > > > @@ -3402,6 +3403,50 @@ static const struct file_operations force_bredr_smp_fops = { > > .llseek = default_llseek, > > }; > > > > +static ssize_t enforce_mitm_smp_read(struct file *file, > > + char __user *user_buf, > > + size_t count, loff_t *ppos) > > +{ > > + struct hci_dev *hdev = file->private_data; > > + char buf[3]; > > + > > + buf[0] = hci_dev_test_flag(hdev, HCI_ENFORCE_MITM_SMP) ? 'Y' : 'N'; > > + buf[1] = '\n'; > > + buf[2] = '\0'; > > + return simple_read_from_buffer(user_buf, count, ppos, buf, 2); > > +} > > + > > +static ssize_t enforce_mitm_smp_write(struct file *file, > > + const char __user *user_buf, > > + size_t count, loff_t *ppos) > > +{ > > + struct hci_dev *hdev = file->private_data; > > + char buf[32]; > > + size_t buf_size = min(count, (sizeof(buf) - 1)); > > + bool enable; > > + > > + if (copy_from_user(buf, user_buf, buf_size)) > > + return -EFAULT; > > + > > + buf[buf_size] = '\0'; > > + if (strtobool(buf, &enable)) > > + return -EINVAL; > > + > > + if (enable == hci_dev_test_flag(hdev, HCI_ENFORCE_MITM_SMP)) > > + return -EALREADY; > > + > > + hci_dev_change_flag(hdev, HCI_ENFORCE_MITM_SMP); > > + > > + return count; > > +} > > + > > +static const struct file_operations enforce_mitm_smp_fops = { > > + .open = simple_open, > > + .read = enforce_mitm_smp_read, > > + .write = enforce_mitm_smp_write, > > + .llseek = default_llseek, > > +}; > > + > > int smp_register(struct hci_dev *hdev) > > { > > struct l2cap_chan *chan; > > @@ -3426,6 +3471,11 @@ int smp_register(struct hci_dev *hdev) > > > > hdev->smp_data = chan; > > > > + /* Enforce the policy of determining MITM flag by io capabilities. */ > > + hci_dev_set_flag(hdev, HCI_ENFORCE_MITM_SMP); > > No. Lets keep the current behavior the default. > > > + debugfs_create_file("enforce_mitm_smp", 0644, hdev->debugfs, hdev, > > + &enforce_mitm_smp_fops); > > + > > And this needs to move into hci_debugfs.c. > > Regards > > Marcel >
Hi Archie, > The way I implemented it is, if HCI_ENFORCE_MITM_SMP is set (which it > is by default), then it will assume the default behavior. > However, if it is toggled to false, then it will not set the MITM flag > although the io capability supports that. > > I am reluctant to use names with "no" on it, especially since it is a > boolean. But if it is OK then I shall update to HCI_FORCE_NO_MITM, > this way it will become more separable with the default behavior. > > Sure, I will move that to hci_debugfs.c. I dislike setting a flag for default behavior. So we need the invert here. I want the “force” in front of it that it clearly indicates that it is not default behavior. Similar to the force_static_addr flag. Regards Marcel
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h index 79de2a659dd69..5e183487c7479 100644 --- a/include/net/bluetooth/hci.h +++ b/include/net/bluetooth/hci.h @@ -298,6 +298,7 @@ enum { HCI_FORCE_STATIC_ADDR, HCI_LL_RPA_RESOLUTION, HCI_CMD_PENDING, + HCI_ENFORCE_MITM_SMP, __HCI_NUM_FLAGS, }; diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c index d0b695ee49f63..4fa8b112fb607 100644 --- a/net/bluetooth/smp.c +++ b/net/bluetooth/smp.c @@ -2396,7 +2396,8 @@ int smp_conn_security(struct hci_conn *hcon, __u8 sec_level) /* Require MITM if IO Capability allows or the security level * requires it. */ - if (hcon->io_capability != HCI_IO_NO_INPUT_OUTPUT || + if ((hci_dev_test_flag(hcon->hdev, HCI_ENFORCE_MITM_SMP) && + hcon->io_capability != HCI_IO_NO_INPUT_OUTPUT) || hcon->pending_sec_level > BT_SECURITY_MEDIUM) authreq |= SMP_AUTH_MITM; @@ -3402,6 +3403,50 @@ static const struct file_operations force_bredr_smp_fops = { .llseek = default_llseek, }; +static ssize_t enforce_mitm_smp_read(struct file *file, + char __user *user_buf, + size_t count, loff_t *ppos) +{ + struct hci_dev *hdev = file->private_data; + char buf[3]; + + buf[0] = hci_dev_test_flag(hdev, HCI_ENFORCE_MITM_SMP) ? 'Y' : 'N'; + buf[1] = '\n'; + buf[2] = '\0'; + return simple_read_from_buffer(user_buf, count, ppos, buf, 2); +} + +static ssize_t enforce_mitm_smp_write(struct file *file, + const char __user *user_buf, + size_t count, loff_t *ppos) +{ + struct hci_dev *hdev = file->private_data; + char buf[32]; + size_t buf_size = min(count, (sizeof(buf) - 1)); + bool enable; + + if (copy_from_user(buf, user_buf, buf_size)) + return -EFAULT; + + buf[buf_size] = '\0'; + if (strtobool(buf, &enable)) + return -EINVAL; + + if (enable == hci_dev_test_flag(hdev, HCI_ENFORCE_MITM_SMP)) + return -EALREADY; + + hci_dev_change_flag(hdev, HCI_ENFORCE_MITM_SMP); + + return count; +} + +static const struct file_operations enforce_mitm_smp_fops = { + .open = simple_open, + .read = enforce_mitm_smp_read, + .write = enforce_mitm_smp_write, + .llseek = default_llseek, +}; + int smp_register(struct hci_dev *hdev) { struct l2cap_chan *chan; @@ -3426,6 +3471,11 @@ int smp_register(struct hci_dev *hdev) hdev->smp_data = chan; + /* Enforce the policy of determining MITM flag by io capabilities. */ + hci_dev_set_flag(hdev, HCI_ENFORCE_MITM_SMP); + debugfs_create_file("enforce_mitm_smp", 0644, hdev->debugfs, hdev, + &enforce_mitm_smp_fops); + /* If the controller does not support BR/EDR Secure Connections * feature, then the BR/EDR SMP channel shall not be present. *