| Message ID | 20190725092253.15912-1-baijiaju1990@gmail.com |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: bluetooth: hci_sock: Fix a possible null-pointer dereference in hci_mgmt_cmd() | expand |
Hi Jia-Ju, > In hci_mgmt_cmd(), there is an if statement on line 1570 to check > whether hdev is NULL: > if (hdev && chan->hdev_init) > > When hdev is NULL, it is used on line 1575: > err = handler->func(sk, hdev, cp, len); > > Some called functions of handler->func use hdev, such as: > set_appearance(), add_device() and remove_device() in mgmt.c. > > Thus, a possible null-pointer dereference may occur. > > To fix this bug, hdev is checked before calling handler->func(). > > This bug is found by a static analysis tool STCheck written by us. > > Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> > --- > net/bluetooth/hci_sock.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c > index d32077b28433..18ea1e47ea48 100644 > --- a/net/bluetooth/hci_sock.c > +++ b/net/bluetooth/hci_sock.c > @@ -1570,11 +1570,12 @@ static int hci_mgmt_cmd(struct hci_mgmt_chan *chan, struct sock *sk, > if (hdev && chan->hdev_init) > chan->hdev_init(sk, hdev); > > - cp = buf + sizeof(*hdr); > - > - err = handler->func(sk, hdev, cp, len); > - if (err < 0) > - goto done; > + if (hdev) { > + cp = buf + sizeof(*hdr); > + err = handler->func(sk, hdev, cp, len); > + if (err < 0) > + goto done; > + } > > err = msglen; have you evaluated the statement above: no_hdev = (handler->flags & HCI_MGMT_NO_HDEV); if (no_hdev != !hdev) { err = mgmt_cmd_status(sk, index, opcode, MGMT_STATUS_INVALID_INDEX); goto done; } I think that code is just overly complex and can be simplified, but I doubt you get to the situation where hdev is NULL for any function that requires it. Only the handler->func marked with HCI_MGMT_NO_HDEV will get hdev == NULL and these are not using it. So we might can make this easier code to really check the index != MGMT_INDEX_NONE check above to cover all cases to ensure that hdev is either valid or set to NULL before proceeding any further. And since we have a full set of unit tests in tools/mgmt-tester, I assume we would have had a chance to catch an issue like this. But we can add a test case to it to explicitly call the functions with either MGMT_INDEX_NONE used or not. Regards Marcel
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c index d32077b28433..18ea1e47ea48 100644 --- a/net/bluetooth/hci_sock.c +++ b/net/bluetooth/hci_sock.c @@ -1570,11 +1570,12 @@ static int hci_mgmt_cmd(struct hci_mgmt_chan *chan, struct sock *sk, if (hdev && chan->hdev_init) chan->hdev_init(sk, hdev); - cp = buf + sizeof(*hdr); - - err = handler->func(sk, hdev, cp, len); - if (err < 0) - goto done; + if (hdev) { + cp = buf + sizeof(*hdr); + err = handler->func(sk, hdev, cp, len); + if (err < 0) + goto done; + } err = msglen;
In hci_mgmt_cmd(), there is an if statement on line 1570 to check whether hdev is NULL: if (hdev && chan->hdev_init) When hdev is NULL, it is used on line 1575: err = handler->func(sk, hdev, cp, len); Some called functions of handler->func use hdev, such as: set_appearance(), add_device() and remove_device() in mgmt.c. Thus, a possible null-pointer dereference may occur. To fix this bug, hdev is checked before calling handler->func(). This bug is found by a static analysis tool STCheck written by us. Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> --- net/bluetooth/hci_sock.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)