linux-kernel - Re: [PATCH 2/2] Bluetooth: btqcomsmd: BD address setup

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:   Fri, 1 Sep 2017 16:26:53 -0500
From:   Rob Herring <robh@...nel.org>
To:     Marcel Holtmann <marcel@...tmann.org>
Cc:     Bjorn Andersson <bjorn.andersson@...aro.org>,
        "Gustavo F. Padovan" <gustavo@...ovan.org>,
        Johan Hedberg <johan.hedberg@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        "open list:BLUETOOTH DRIVERS" <linux-bluetooth@...r.kernel.org>,
        Network Development <netdev@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        Loic Poulain <loic.poulain@...aro.org>
Subject: Re: [PATCH 2/2] Bluetooth: btqcomsmd: BD address setup

On Fri, Sep 1, 2017 at 3:47 PM, Marcel Holtmann <marcel@...tmann.org> wrote:
> Hi Bjorn,
>
>> Bluetooth BD address can be retrieved in the same way as
>> for wcnss-wlan MAC address. This patch mainly stores the
>> local-mac-address property and sets the BD address during
>> hci device setup.
>>
>> Signed-off-by: Loic Poulain <loic.poulain@...aro.org>
>> Signed-off-by: Bjorn Andersson <bjorn.andersson@...aro.org>
>> ---
>> drivers/bluetooth/btqcomsmd.c | 28 ++++++++++++++++++++++++++++
>> 1 file changed, 28 insertions(+)
>>
>> diff --git a/drivers/bluetooth/btqcomsmd.c b/drivers/bluetooth/btqcomsmd.c
>> index d00c4fdae924..443bb2099329 100644
>> --- a/drivers/bluetooth/btqcomsmd.c
>> +++ b/drivers/bluetooth/btqcomsmd.c
>> @@ -26,6 +26,7 @@
>> struct btqcomsmd {
>>       struct hci_dev *hdev;
>>
>> +     const bdaddr_t *addr;
>>       struct rpmsg_endpoint *acl_channel;
>>       struct rpmsg_endpoint *cmd_channel;
>> };
>> @@ -100,6 +101,27 @@ static int btqcomsmd_close(struct hci_dev *hdev)
>>       return 0;
>> }
>>
>> +static int btqcomsmd_setup(struct hci_dev *hdev)
>> +{
>> +     struct btqcomsmd *btq = hci_get_drvdata(hdev);
>> +     struct sk_buff *skb;
>> +
>> +     skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_TIMEOUT);
>> +     if (IS_ERR(skb))
>> +             return PTR_ERR(skb);
>> +     kfree_skb(skb);
>> +
>> +     if (btq->addr) {
>> +             bdaddr_t bdaddr;
>> +
>> +             /* btq->addr stored with most significant byte first */
>> +             baswap(&bdaddr, btq->addr);
>> +             return qca_set_bdaddr_rome(hdev, &bdaddr);
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> static int btqcomsmd_probe(struct platform_device *pdev)
>> {
>>       struct btqcomsmd *btq;
>> @@ -123,6 +145,11 @@ static int btqcomsmd_probe(struct platform_device *pdev)
>>       if (IS_ERR(btq->cmd_channel))
>>               return PTR_ERR(btq->cmd_channel);
>>
>> +     btq->addr = of_get_property(pdev->dev.of_node, "local-mac-address",
>> +                                 &ret);
>> +     if (ret != sizeof(bdaddr_t))
>> +             btq->addr = NULL;
>> +
>>       hdev = hci_alloc_dev();
>>       if (!hdev)
>>               return -ENOMEM;
>> @@ -135,6 +162,7 @@ static int btqcomsmd_probe(struct platform_device *pdev)
>>       hdev->open = btqcomsmd_open;
>>       hdev->close = btqcomsmd_close;
>>       hdev->send = btqcomsmd_send;
>> +     hdev->setup = btqcomsmd_setup;
>>       hdev->set_bdaddr = qca_set_bdaddr_rome;
>
> I do not like this patch. Why not just set HCI_QUIRK_INVALID_BDADDR and let a userspace tool deal with reading the BD_ADDR from some storage.
>
> Frankly I do not get this WiFI MAC address or BD_ADDR stored in DT. I assumed the DT is suppose to describe hardware and not some value that is normally retrieved for OTP or alike.

Use of "local-mac-address" for ethernet at least has existed as long
at OpenFirmware I think. For some platforms, DT is the only OTP. And
sometimes, the bootloader (like u-boot) stores MAC addresses and then
populates them on boot.

Seems like if we just let userspace deal with it, then we're back to a
btattach tool with every platform's specific way of reading the MAC
address.

Rob