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@xxxxxxxxx> Sent: Friday, June 28, 2024 10:29 PM To: Tsunashima Nobuaki (SMD C3 JP RM WLS AE) <Nobuaki.Tsunashima@xxxxxxxxxxxx> Cc: marcel@xxxxxxxxxxxx; linux-bluetooth@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx 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@xxxxxxxxxxxx> 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
