RE: [PATCH v4] Bluetooth: btbcm: Apply HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER to CYW4373

[<Nobuaki.Tsunashima@infineon.com>]

Hi Luiz,

Thanks for your comment.

> Ok, but I still consider reworking these to use skb_pull_data.

Now, I reconsider and found the skb_pull_data is more convenient rather than directly accessing to skb->data.
As I am on business trip on a few days, I will submit new patch after I come back.

Regards,
Nobuaki Tsunashima

-----Original Message-----
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com> 
Sent: Friday, June 28, 2024 10:29 PM
To: Tsunashima Nobuaki (SMD C3 JP RM WLS AE) <Nobuaki.Tsunashima@infineon.com>
Cc: marcel@holtmann.org; linux-bluetooth@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4] Bluetooth: btbcm: Apply HCI_QUIRK_BROKEN_READ_TRANSMIT_POWER to CYW4373

Caution: This e-mail originated outside Infineon Technologies. Please be cautious when sharing information or opening attachments especially from unknown senders. Refer to our intranet guide<https://intranet-content.infineon.com/explore/aboutinfineon/rules/informationsecurity/ug/SocialEngineering/Pages/SocialEngineeringElements_en.aspx> to help you identify Phishing email.



Hi,

On Sun, May 26, 2024 at 9:59 PM <Nobuaki.Tsunashima@infineon.com> wrote:
>
> Hi Luiz,
>
> Thanks for your review.
>
> >>  static int btbcm_read_info(struct hci_dev *hdev)  {
> >>         struct sk_buff *skb;
> >> +       u8 chip_id;
> >> +       u16 baseline;
> >>
> >>         /* Read Verbose Config Version Info */
> >>         skb = btbcm_read_verbose_config(hdev);
> >>         if (IS_ERR(skb))
> >>                 return PTR_ERR(skb);
> >> -
> >> +       chip_id = skb->data[1];
> >> +       baseline = skb->data[3] | (skb->data[4] << 8);
> >
> >This is not really safe, you shouldn't attempt to access skb->data without first checking skb->len, actually it would be much better that >you would use skb_pull_data which does skb->len check before pulling data.
>
> I think it could be safe because its length is checked inside btbcm_read_verbose_config() as below.
> Please let me know if further checking is needed.
>
> >>>
> static struct sk_buff *btbcm_read_verbose_config(struct hci_dev *hdev) 
> {
>         struct sk_buff *skb;
>
>         skb = __hci_cmd_sync(hdev, 0xfc79, 0, NULL, HCI_INIT_TIMEOUT);
>         if (IS_ERR(skb)) {
>                 bt_dev_err(hdev, "BCM: Read verbose config info failed (%ld)",
>                            PTR_ERR(skb));
>                 return skb;
>         }
>
>         if (skb->len != 7) {
>                 bt_dev_err(hdev, "BCM: Verbose config length mismatch");
>                 kfree_skb(skb);
>                 return ERR_PTR(-EIO);
>         }
>
>         return skb;
> }
> <<<

Ok, but I still consider reworking these to use skb_pull_data.

> Best Regards,
> Nobuaki Tsunashima
>


--
Luiz Augusto von Dentz