Hi Marcel, On Fri, Oct 18, 2013, Marcel Holtmann wrote: > > Old user space versions bind the Attribute Protocol socket to > > BDADDR_BREDR when they should be using BDADDR_LE_PUBLIC or > > BDADDR_LE_RANDOM. The kernel recently introduced stricter checks on the > > socket parameters but we need to punch this hole to them to ensure that > > old user space versions keep working. > > > > Signed-off-by: Johan Hedberg <johan.hedberg@xxxxxxxxx> > > --- > > net/bluetooth/l2cap_sock.c | 19 +++++++++++++++++-- > > 1 file changed, 17 insertions(+), 2 deletions(-) > > > > diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c > > index 34e5a58..a43aead 100644 > > --- a/net/bluetooth/l2cap_sock.c > > +++ b/net/bluetooth/l2cap_sock.c > > @@ -159,8 +159,23 @@ static int l2cap_sock_connect(struct socket *sock, struct sockaddr *addr, > > if (!bdaddr_type_is_valid(la.l2_bdaddr_type)) > > return -EINVAL; > > > > - if (chan->src_type == BDADDR_BREDR && la.l2_bdaddr_type != BDADDR_BREDR) > > - return -EINVAL; > > + if (chan->src_type == BDADDR_BREDR && > > + la.l2_bdaddr_type != BDADDR_BREDR) { > > + if (!bdaddr_type_is_le(la.l2_bdaddr_type)) > > + return -EINVAL; > > I do not understand this. The check for that la.l2_bdaddr_type is LE > is done twice here. The idea was simply to try to be forward compatible in case a fourth address type is added, i.e. I made the assumption that "a != BDADDR_BREDR" might not always be the same as "bdaddr_type_is_le(a)". If you're ok with this assumption we should not just remove this extra if-statement but also change the higher-level one to use bdaddr_type_is_le() instead of making explicit comparisons with BDADDR_BREDR. > > + > > + /* Old user space versions will try to bind the ATT > > + * socket using BDADDR_BREDR, so we have to allow this > > + * to pass and fix chan->src_type to the correct value. > > + */ > > + if (la.l2_cid != __constant_cpu_to_le16(L2CAP_CID_ATT)) > > + return -EINVAL; > > I think we should also check that the src address is BDADDR_ANY. If it > is not BDADDR_ANY, we should reject it as well. That is enough to make > old userspace work. I do not want us to be any more flexible. Or are > we actually bind to the right source adapter when calling connect(). The user space code is correct in this regard, so I'll add the extra check for BDADDR_ANY. > > + if (chan->scid != L2CAP_CID_ATT) > > + return -EINVAL; > > + > > + chan->src_type = BDADDR_LE_PUBLIC; > > + } > > I do not like the fact that we are just defaulting to BDADDR_LE_PUBLIC > here. It might be fine since the old userspace only ever understood > public addresses anyway. However I do not like it that much. And if we > keep it this way, then a comment needs to explain that this is good > enough. I'll add a comment. We could in theory extend the code to also check for information inside the hdev struct, but that would require too much complexity for the benefit since we do not have the hdev available at this point in the socket operations. That, considered together with the fact that even now user space will have a hard-coded BDADDR_LE_PUBLIC is imo reason enough to just keep this simple in the kernel. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html