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
