| Message ID | 20211115085613.1924762-1-liu.yun@linux.dev (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | bluetooth: fix uninitialized variables notify_evt | expand |
Hi Jackie, > Coverity Scan report: > > [...] > *** CID 1493985: Uninitialized variables (UNINIT) > /net/bluetooth/hci_event.c: 4535 in hci_sync_conn_complete_evt() > 4529 > 4530 /* Notify only in case of SCO over HCI transport data path which > 4531 * is zero and non-zero value shall be non-HCI transport data path > 4532 */ > 4533 if (conn->codec.data_path == 0) { > 4534 if (hdev->notify) >>>> CID 1493985: Uninitialized variables (UNINIT) >>>> Using uninitialized value "notify_evt" when calling "*hdev->notify". > 4535 hdev->notify(hdev, notify_evt); > 4536 } > 4537 > 4538 hci_connect_cfm(conn, ev->status); > 4539 if (ev->status) > 4540 hci_conn_del(conn); > [...] > > Although only btusb uses air_mode, and he only handles HCI_NOTIFY_ENABLE_SCO_CVSD > and HCI_NOTIFY_ENABLE_SCO_TRANSP, there is still a very small chance that > ev->air_mode is not equal to 0x2 and 0x3, but notify_evt is initialized to > HCI_NOTIFY_ENABLE_SCO_CVSD or HCI_NOTIFY_ENABLE_SCO_TRANSP. the context is > maybe not correct. > > In order to ensure 100% correctness, we directly give him a default value 0. > > Addresses-Coverity: ("Uninitialized variables") > Fixes: f4f9fa0c07bb ("Bluetooth: Allow usb to auto-suspend when SCO use non-HCI transport") > Signed-off-by: Jackie Liu <liuyun01@kylinos.cn> > --- > net/bluetooth/hci_event.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c > index 7d0db1ca1248..f898fa42a183 100644 > --- a/net/bluetooth/hci_event.c > +++ b/net/bluetooth/hci_event.c > @@ -4445,7 +4445,7 @@ static void hci_sync_conn_complete_evt(struct hci_dev *hdev, > { > struct hci_ev_sync_conn_complete *ev = (void *) skb->data; > struct hci_conn *conn; > - unsigned int notify_evt; > + unsigned int notify_evt = 0; > > BT_DBG("%s status 0x%2.2x", hdev->name, ev->status); lets modify the switch statement and add a default case. And then lets add a check to notify_evt != 0. With that in mind, I wonder if this is not better /* Notify only in case of SCO over HCI transport data path which * is zero and non-zero value shall be non-HCI transport data path */ if (conn->codec.data_path == 0 && hdev->notify) { switch (ev->air_mode) { case 0x02: hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_CVSD); break; case 0x03: hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_TRANSP); break; } } Regards Marcel
Hi, Marcel. 在 2021/11/16 上午1:27, Marcel Holtmann 写道: > Hi Jackie, > >> Coverity Scan report: >> >> [...] >> *** CID 1493985: Uninitialized variables (UNINIT) >> /net/bluetooth/hci_event.c: 4535 in hci_sync_conn_complete_evt() >> 4529 >> 4530 /* Notify only in case of SCO over HCI transport data path which >> 4531 * is zero and non-zero value shall be non-HCI transport data path >> 4532 */ >> 4533 if (conn->codec.data_path == 0) { >> 4534 if (hdev->notify) >>>>> CID 1493985: Uninitialized variables (UNINIT) >>>>> Using uninitialized value "notify_evt" when calling "*hdev->notify". >> 4535 hdev->notify(hdev, notify_evt); >> 4536 } >> 4537 >> 4538 hci_connect_cfm(conn, ev->status); >> 4539 if (ev->status) >> 4540 hci_conn_del(conn); >> [...] >> >> Although only btusb uses air_mode, and he only handles HCI_NOTIFY_ENABLE_SCO_CVSD >> and HCI_NOTIFY_ENABLE_SCO_TRANSP, there is still a very small chance that >> ev->air_mode is not equal to 0x2 and 0x3, but notify_evt is initialized to >> HCI_NOTIFY_ENABLE_SCO_CVSD or HCI_NOTIFY_ENABLE_SCO_TRANSP. the context is >> maybe not correct. >> >> In order to ensure 100% correctness, we directly give him a default value 0. >> >> Addresses-Coverity: ("Uninitialized variables") >> Fixes: f4f9fa0c07bb ("Bluetooth: Allow usb to auto-suspend when SCO use non-HCI transport") >> Signed-off-by: Jackie Liu <liuyun01@kylinos.cn> >> --- >> net/bluetooth/hci_event.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c >> index 7d0db1ca1248..f898fa42a183 100644 >> --- a/net/bluetooth/hci_event.c >> +++ b/net/bluetooth/hci_event.c >> @@ -4445,7 +4445,7 @@ static void hci_sync_conn_complete_evt(struct hci_dev *hdev, >> { >> struct hci_ev_sync_conn_complete *ev = (void *) skb->data; >> struct hci_conn *conn; >> - unsigned int notify_evt; >> + unsigned int notify_evt = 0; >> >> BT_DBG("%s status 0x%2.2x", hdev->name, ev->status); > > lets modify the switch statement and add a default case. And then lets add a check to notify_evt != 0. > > With that in mind, I wonder if this is not better > > /* Notify only in case of SCO over HCI transport data path which > * is zero and non-zero value shall be non-HCI transport data path > */ > if (conn->codec.data_path == 0 && hdev->notify) { > switch (ev->air_mode) { > case 0x02: > hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_CVSD); > break; > case 0x03: > hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_TRANSP); > break; > } > } I prefer this, because it is a restoration of earlier logic, rather than changing his logic. I will resend a patch, thank you. -- Jackie Liu > > Regards > > Marcel >
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c index 7d0db1ca1248..f898fa42a183 100644 --- a/net/bluetooth/hci_event.c +++ b/net/bluetooth/hci_event.c @@ -4445,7 +4445,7 @@ static void hci_sync_conn_complete_evt(struct hci_dev *hdev, { struct hci_ev_sync_conn_complete *ev = (void *) skb->data; struct hci_conn *conn; - unsigned int notify_evt; + unsigned int notify_evt = 0; BT_DBG("%s status 0x%2.2x", hdev->name, ev->status);