From: Johan Hedberg <[email protected]>
The bnep_get_device function may be triggered by an ioctl just after a
connection has gone down. In such a case the respective L2CAP chan->conn
pointer will get set to NULL (by l2cap_chan_del). This patch adds a
missing NULL check for this case in the bnep_get_device() function.
Reported-by: Patrik Flykt <[email protected]>
Signed-off-by: Johan Hedberg <[email protected]>
---
net/bluetooth/bnep/core.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/net/bluetooth/bnep/core.c b/net/bluetooth/bnep/core.c
index ce82722d049b..05f57e491ccb 100644
--- a/net/bluetooth/bnep/core.c
+++ b/net/bluetooth/bnep/core.c
@@ -511,13 +511,12 @@ static int bnep_session(void *arg)
static struct device *bnep_get_device(struct bnep_session *session)
{
- struct hci_conn *conn;
+ struct l2cap_conn *conn = l2cap_pi(session->sock->sk)->chan->conn;
- conn = l2cap_pi(session->sock->sk)->chan->conn->hcon;
- if (!conn)
+ if (!conn || !conn->hcon)
return NULL;
- return &conn->dev;
+ return &conn->hcon->dev;
}
static struct device_type bnep_type = {
--
2.1.0
Hi Johan,
> The bnep_get_device function may be triggered by an ioctl just after a
> connection has gone down. In such a case the respective L2CAP chan->conn
> pointer will get set to NULL (by l2cap_chan_del). This patch adds a
> missing NULL check for this case in the bnep_get_device() function.
>
> Reported-by: Patrik Flykt <[email protected]>
> Signed-off-by: Johan Hedberg <[email protected]>
> ---
> net/bluetooth/bnep/core.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
patch has been applied to bluetooth-next tree.
Regards
Marcel
SGkgTWFyY2VsLA0KDQo+Pj4+IGFkZHIgY2FuIGJlIE5VTEwgYW5kIGl0IHNob3VsZCBub3QgYmUg
ZGVyZWZlcmVuY2VkIGJlZm9yZSBOVUxMIGNoZWNraW5nLg0KPj4+PiANCj4+Pj4gU2lnbmVkLW9m
Zi1ieTogSmFnYW5hdGggS2FuYWtrYXNzZXJ5IDxqYWdhbmF0aC5rQHNhbXN1bmcuY29tPg0KPj4+
PiAtLS0NCj4+ID4NCj4+ID5pZiB3ZSBzdGFydCBjaGFuZ2luZyB0aGluZ3MgaGVyZSwgdGhlbiB3
ZSBiZXR0ZXIgY2hhbmdlIHRoZSBjb2RlIGludG8gc29tZXRoaW5nIHRoYXQgYWxsIHRoZSBvdGhl
ciBzb2NrZXQgaGFuZGxpbmcgY29kZSBpcyBkb2luZyBhbnl3YXk+eS4gU28gZG8gdGhlIG1pbiBj
b21wYXJpc29uIGFuZCBjb3B5IHRoZSBkYXRhIGludG8gYSBsb2NhbCBjb3B5IG9mIHRoZSBzb2Nr
YWRkcl9yYy4NCj4+Pg0KPj4+IEFuZCBvbiBhIHNpZGUgbm90ZSwgSSB3b25kZXIgaWYgYWRkciBj
YW4gYWN0dWFsbHkgYmUgTlVMTC4gSXQgbWlnaHQgYmUgaW50ZXJlc3RpbmcgdG8gY2hlY2sgdGhl
IGdlbmVyaWMgc29ja2V0IGNvZGUgaWYgdGhpcyByZWFsbHkgY2FuIGhhcHBlPm4gaWYgeW91IHBy
b3ZpZGUgbm8gYWRkcmVzcyBzdHJ1Y3R1cmUgdG8gdGhlIGJpbmQoKSBzeXN0ZW0gY2FsbCBvciBp
ZiB0aGlzIGdldHMgZmlsdGVyZWQgb3V0IGJ5IHRoZSBjb3JlIHNvY2tldCBjb2RlLg0KPj4gDQo+
PiBJIGNoZWNrZWQgZ2VuZXJpYyBzb2NrZXQgY29kZSBhbmQgaXQgbG9va3MgbGlrZSBhZGRyIHdp
bGwgbmV2ZXIgYmUgTlVMTCB3aGVuIHVzZXIgc3BhY2UgY2FsbHMgYmluZC4NCj4+IEJ1dCB0aGlz
IGNhbiBiZSBjYWxsZWQgZnJvbSBrZXJuZWxfYmluZCgpIGFsc28gd2hpY2ggSSB0aGluayB3aWxs
IG5ldmVyIGJlIGNhbGxlZCBmb3IgUkZDT01NLg0KPj4gU28gdGhpcyBwYXRjaCBpcyBub3QgcmVx
dWlyZWQ/IA0KPg0KPnRoYXQgaXMgd2hhdCBJIHRob3VnaHQuIEhvd2V2ZXIgY29udmVydGluZyBp
dCB0byB0aGUgc2FtZSBoYW5kbGluZyB1c2luZyBtaW4gYW5kIGNvcHlpbmcgaW50byBsb2NhbCBz
dG9yYWdlIG1pZ2h0IGJlIGEgZ29vZCBpZGVhLiBUaGUgbW9yZSBwaWVjZXMgaW4gSENJLCBMMkNB
UCwgU0NPIGFuZCBSRkNPTU0gc29ja2V0cyB0aGF0IGFyZSBzaW1pbGFyLCB0aGUgYmV0dGVyLg0K
DQpJIGhhdmUgcmFpc2VkIHYxIHdpdGggdGhlIGNoYW5nZXMgeW91IHN1Z2dlc3RlZCwgUGx6IGNo
ZWNrIGl0Lg0KDQpUaGFua3MsDQpKYWdhbmF0aA==
Hi Jaganath,
>>> addr can be NULL and it should not be dereferenced before NULL checking.
>>>
>>> Signed-off-by: Jaganath Kanakkassery <[email protected]>
>>> ---
>>
>> if we start changing things here, then we better change the code into something that all the other socket handling code is doing anyway>y. So do the min comparison and copy the data into a local copy of the sockaddr_rc.
>>
>> And on a side note, I wonder if addr can actually be NULL. It might be interesting to check the generic socket code if this really can happe>n if you provide no address structure to the bind() system call or if this gets filtered out by the core socket code.
>
> I checked generic socket code and it looks like addr will never be NULL when user space calls bind.
> But this can be called from kernel_bind() also which I think will never be called for RFCOMM.
> So this patch is not required?
that is what I thought. However converting it to the same handling using min and copying into local storage might be a good idea. The more pieces in HCI, L2CAP, SCO and RFCOMM sockets that are similar, the better.
Regards
Marcel
SGkgTWFyY2VsLA0KDQo+PiBhZGRyIGNhbiBiZSBOVUxMIGFuZCBpdCBzaG91bGQgbm90IGJlIGRl
cmVmZXJlbmNlZCBiZWZvcmUgTlVMTCBjaGVja2luZy4NCj4+IA0KPj4gU2lnbmVkLW9mZi1ieTog
SmFnYW5hdGggS2FuYWtrYXNzZXJ5IDxqYWdhbmF0aC5rQHNhbXN1bmcuY29tPg0KPj4gLS0tDQo+
DQo+aWYgd2Ugc3RhcnQgY2hhbmdpbmcgdGhpbmdzIGhlcmUsIHRoZW4gd2UgYmV0dGVyIGNoYW5n
ZSB0aGUgY29kZSBpbnRvIHNvbWV0aGluZyB0aGF0IGFsbCB0aGUgb3RoZXIgc29ja2V0IGhhbmRs
aW5nIGNvZGUgaXMgZG9pbmcgYW55d2F5PnkuIFNvIGRvIHRoZSBtaW4gY29tcGFyaXNvbiBhbmQg
Y29weSB0aGUgZGF0YSBpbnRvIGEgbG9jYWwgY29weSBvZiB0aGUgc29ja2FkZHJfcmMuDQo+DQo+
QW5kIG9uIGEgc2lkZSBub3RlLCBJIHdvbmRlciBpZiBhZGRyIGNhbiBhY3R1YWxseSBiZSBOVUxM
LiBJdCBtaWdodCBiZSBpbnRlcmVzdGluZyB0byBjaGVjayB0aGUgZ2VuZXJpYyBzb2NrZXQgY29k
ZSBpZiB0aGlzIHJlYWxseSBjYW4gaGFwcGU+biBpZiB5b3UgcHJvdmlkZSBubyBhZGRyZXNzIHN0
cnVjdHVyZSB0byB0aGUgYmluZCgpIHN5c3RlbSBjYWxsIG9yIGlmIHRoaXMgZ2V0cyBmaWx0ZXJl
ZCBvdXQgYnkgdGhlIGNvcmUgc29ja2V0IGNvZGUuDQoNCkkgY2hlY2tlZCBnZW5lcmljIHNvY2tl
dCBjb2RlIGFuZCBpdCBsb29rcyBsaWtlIGFkZHIgd2lsbCBuZXZlciBiZSBOVUxMIHdoZW4gdXNl
ciBzcGFjZSBjYWxscyBiaW5kLg0KQnV0IHRoaXMgY2FuIGJlIGNhbGxlZCBmcm9tIGtlcm5lbF9i
aW5kKCkgYWxzbyB3aGljaCBJIHRoaW5rIHdpbGwgbmV2ZXIgYmUgY2FsbGVkIGZvciBSRkNPTU0u
DQpTbyB0aGlzIHBhdGNoIGlzIG5vdCByZXF1aXJlZD8gDQoNClRoYW5rcywNCkphZ2FuYXRo
Hi Jaganath,
> addr can be NULL and it should not be dereferenced before NULL checking.
>
> Signed-off-by: Jaganath Kanakkassery <[email protected]>
> ---
> net/bluetooth/rfcomm/sock.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
> index 825e8fb..e576374 100644
> --- a/net/bluetooth/rfcomm/sock.c
> +++ b/net/bluetooth/rfcomm/sock.c
> @@ -336,14 +336,16 @@ static int rfcomm_sock_bind(struct socket *sock, struct sockaddr *addr, int addr
> {
> struct sockaddr_rc *sa = (struct sockaddr_rc *) addr;
> struct sock *sk = sock->sk;
> - int chan = sa->rc_channel;
> + int chan;
> int err = 0;
>
> - BT_DBG("sk %p %pMR", sk, &sa->rc_bdaddr);
> -
> if (!addr || addr->sa_family != AF_BLUETOOTH)
> return -EINVAL;
>
> + BT_DBG("sk %p %pMR", sk, &sa->rc_bdaddr);
> +
> + chan = sa->rc_channel;
> +
if we start changing things here, then we better change the code into something that all the other socket handling code is doing anyway. So do the min comparison and copy the data into a local copy of the sockaddr_rc.
And on a side note, I wonder if addr can actually be NULL. It might be interesting to check the generic socket code if this really can happen if you provide no address structure to the bind() system call or if this gets filtered out by the core socket code.
Regards
Marcel
Hi Marcel,
On Thu, 2015-05-14 at 08:34 +0200, Marcel Holtmann wrote:
> Hi Jaganath,
>
> > > > addr can be NULL and it should not be dereferenced before NULL
> > > > checking.
> > > >
> > > > Signed-off-by: Jaganath Kanakkassery <[email protected]>
> > > > ---
> > >
> > > if we start changing things here, then we better change the code
> > > into something that all the other socket handling code is doing
> > > anyway>y. So do the min comparison and copy the data into a local
> > > copy of the sockaddr_rc.
> > >
> > > And on a side note, I wonder if addr can actually be NULL. It
> > > might be interesting to check the generic socket code if this
> > > really can happe>n if you provide no address structure to the
> > > bind() system call or if this gets filtered out by the core
> > > socket code.
> >
> > I checked generic socket code and it looks like addr will never be
> > NULL when user space calls bind.
> > But this can be called from kernel_bind() also which I think will
> > never be called for RFCOMM.
> > So this patch is not required?
>
> that is what I thought. However converting it to the same handling
> using min and copying into local storage might be a good idea.
Could you tell us why this is good idea? I failed to find it in git
history/mailing list.
In addition to RFCOMM connect that you mentioned, I found out SCO
connect/bind still use the old style in Bluetooth unlikely HCI,L2CAP.
Regards
Chanyeol
> The more pieces in HCI, L2CAP, SCO and RFCOMM sockets that are
> similar, the better.
>
> Regards
>
> Marcel
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux
> -bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Ping.
On Fri, May 15, 2015 at 11:07 AM, JAGANATH KANAKKASSERY <
[email protected]> wrote:
> Hi Marcel,
>
> >>>> addr can be NULL and it should not be dereferenced before NULL
> checking.
> >>>>
> >>>> Signed-off-by: Jaganath Kanakkassery <[email protected]>
> >>>> ---
> >> >
> >> >if we start changing things here, then we better change the code into
> something that all the other socket handling code is doing anyway>y. So do
> the min comparison and copy the data into a local copy of the sockaddr_rc.
> >>>
> >>> And on a side note, I wonder if addr can actually be NULL. It might be
> interesting to check the generic socket code if this really can happe>n if
> you provide no address structure to the bind() system call or if this gets
> filtered out by the core socket code.
> >>
> >> I checked generic socket code and it looks like addr will never be NULL
> when user space calls bind.
> >> But this can be called from kernel_bind() also which I think will never
> be called for RFCOMM.
> >> So this patch is not required?
> >
> >that is what I thought. However converting it to the same handling using
> min and copying into local storage might be a good idea. The more pieces in
> HCI, L2CAP, SCO and RFCOMM sockets that are similar, the better.
>
> I have raised v1 with the changes you suggested, Plz check it.
>
> Thanks,
> Jaganath