linux-kernel - Re: [PATCH v2] Bluetooth: hci_qca: Add support for controller debug logs.

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: <01616379-E0FF-49AC-8C69-DA498DA9F51F@holtmann.org>
Date:   Tue, 16 Oct 2018 17:09:34 +0200
From:   Marcel Holtmann <marcel@...tmann.org>
To:     Balakrishna Godavarthi <bgodavar@...eaurora.org>
Cc:     Johan Hedberg <johan.hedberg@...il.com>,
        Matthias Kaehlcke <mka@...omium.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-bluetooth@...r.kernel.org, hemantg@...eaurora.org,
        linux-arm-msm@...r.kernel.org,
        Harish Bandi <c-hbandi@...eaurora.org>
Subject: Re: [PATCH v2] Bluetooth: hci_qca: Add support for controller debug
 logs.

Hi Balakrishna,

>>>>> This patch will prevent error messages splashing on console.
>>>>> [   78.426697] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
>>>>> [   78.436682] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
>>>>> [   78.446639] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
>>>>> [   78.456596] Bluetooth: hci_core.c:hci_acldata_packet() hci0: ACL packet for unknown connection handle 3804
>>>>> QCA wcn3990 will send the debug logs in the form of ACL packets.
>>>>> While decoding packet in qca_recv(), marking the received debug log
>>>>> packet as diagnostic packet.
>>>>> Signed-off-by: Harish Bandi <c-hbandi@...eaurora.org>
>>>>> Signed-off-by: Balakrishna Godavarthi <bgodavar@...eaurora.org>
>>>>> ---
>>>>> v2:
>>>>> * Addressed reviewer comments.
>>>>> v1:
>>>>> * initial patch
>>>>> ---
>>>>> drivers/bluetooth/hci_qca.c | 20 +++++++++++++++++++-
>>>>> 1 file changed, 19 insertions(+), 1 deletion(-)
>>>>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>>>>> index d98ed0442201..63ac3c6b4149 100644
>>>>> --- a/drivers/bluetooth/hci_qca.c
>>>>> +++ b/drivers/bluetooth/hci_qca.c
>>>>> @@ -63,6 +63,10 @@
>>>>> /* susclk rate */
>>>>> #define SUSCLK_RATE_32KHZ	32768
>>>>> +/* Controller debug log header */
>>>>> +#define QCA_DEBUG_HDR_MSB	0xDC
>>>>> +#define QCA_DEBUG_HDR_LSB	0x2E
>>>>> +
>>>> since this is actually the ACL handle, why not call it QCA_DEBUG_HANDLE.
>>> [Bala]: will update.
>>>>> /* HCI_IBS transmit side sleep protocol states */
>>>>> enum tx_ibs_states {
>>>>> 	HCI_IBS_TX_ASLEEP,
>>>>> @@ -849,6 +853,20 @@ static int qca_ibs_wake_ack(struct hci_dev *hdev, struct sk_buff *skb)
>>>>> 	return 0;
>>>>> }
>>>>> +static int qca_recv_acl_data(struct hci_dev *hdev, struct sk_buff *skb)
>>>>> +{
>>>>> +	/* We receive debug logs from chip as an ACL packets.
>>>>> +	 * Instead of sending the data to ACL to decode the
>>>>> +	 * received data, we are pushing them to the above layers
>>>>> +	 * as a diagnostic packet.
>>>>> +	 */
>>>>> +	if ((skb->len > 2) && (skb->data[0] == QCA_DEBUG_HDR_MSB) &&
>>>>> +	    (skb->data[1] == QCA_DEBUG_HDR_LSB))
>>>> Skip the individual () since they are not needed. Also the skb->len
>>>> check is not needed since the H4_RECV_ACL already ensures the proper
>>>> length of the header.
>>>> And just use get_unaligned_le16(skb->data) here (or be16 if that is
>>>> the byte order).
>>> [Bala] : will update condition with _le16()
>>>>> +		return hci_recv_diag(hdev, skb);
>>>> Is there any reason to keep the 4 octets hci_acl_hdr with this SKB? Or
>>>> would it be better to be stripped off. Mainly asking are they any
>>>> other magic handles that we might want to feed through the diag
>>>> channel?
>>> [Bala]: yes we need header in the stack, to  differentiate between actual diagnostic packet and debug packet.
>> please explain that a bit more. There is just one debug handle or do
>> you have more special handles?
> [Bala]: Qualcomm bluetooth chip sends the debug data of the chip with ACL header along with fixed handler i.e. 0x2edc..
>        this handler is dedicated for the debug logs sent by qcomm bt chip and it is not used for any other connection handlers.
>        so here we are checking this handler and sending to stack as an
>        a diagnostic packet.. so during the log capture we will read this diagnostic packet and handler,to decode the debug packet.
>        that is the  reason why we don't remove handler during condition check,  in future, if qualcomm chip sends diagnostic packet,
>        then this will create an problem to upper layers to decide whether it is an diagnostic packet or the debug packet.
> 
>       this is how we do it.
>      if( diagnostic packet && handler == 0x2edc)
>         treat them as an ddebug packet
>      else
>          treat as diagnostic packet.
> 

let me interpret this answer. Besides debug handle 0x2edc, there are other handles that are not connection handles. And in future updates you want to send these handles also via hci_recv_diag channel.

Regards

Marcel