[PATCH 1/3] Bluetooth: Fix local name in scan rsp

[PATCH 1/3] Bluetooth: Fix local name in scan rsp

[Michał Narajowski]

Use complete name if it fits. If not and there is short name
check if it fits. If not then use shortened name as prefix
of complete name.

Signed-off-by: Michał Narajowski <michal.narajowski@codecoup.pl>
---
 net/bluetooth/hci_request.c | 46 +++++++++++++++++++++++++++++++++------------
 1 file changed, 34 insertions(+), 12 deletions(-)

diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
index c813568..880758c 100644
--- a/net/bluetooth/hci_request.c
+++ b/net/bluetooth/hci_request.c
@@ -973,25 +973,47 @@ void __hci_req_enable_advertising(struct hci_request *req)
 
 static u8 append_local_name(struct hci_dev *hdev, u8 *ptr, u8 ad_len)
 {
-	size_t name_len;
+	size_t complete_len;
+	size_t short_len;
 	int max_len;
 
 	max_len = HCI_MAX_AD_LENGTH - ad_len - 2;
-	name_len = strlen(hdev->dev_name);
-	if (name_len > 0 && max_len > 0) {
+	complete_len = strlen(hdev->dev_name);
+	short_len = strlen(hdev->short_name);
 
-		if (name_len > max_len) {
-			name_len = max_len;
-			ptr[1] = EIR_NAME_SHORT;
-		} else
-			ptr[1] = EIR_NAME_COMPLETE;
+	/* no space left for name */
+	if (max_len < 1)
+		return ad_len;
 
-		ptr[0] = name_len + 1;
+	/* no name set */
+	if (!complete_len)
+		return ad_len;
 
-		memcpy(ptr + 2, hdev->dev_name, name_len);
+	/* complete name fits */
+	if (complete_len <= max_len) {
+		ptr[0] = complete_len + 1;
+		ptr[1] = EIR_NAME_COMPLETE;
+		memcpy(ptr + 2, hdev->dev_name, complete_len);
 
-		ad_len += (name_len + 2);
-		ptr += (name_len + 2);
+		return ad_len + complete_len + 2;
+	}
+
+	/* shortened name set and fits */
+	if (short_len && short_len <= max_len) {
+		ptr[0] = short_len + 1;
+		ptr[1] = EIR_NAME_SHORT;
+		memcpy(ptr + 2, hdev->short_name, short_len);
+
+		return ad_len + short_len + 2;
+	}
+
+	/* no shortened name set so shorten complete name */
+	if (!short_len) {
+		ptr[0] = max_len + 1;
+		ptr[1] = EIR_NAME_SHORT;
+		memcpy(ptr + 2, hdev->dev_name, max_len);
+
+		return ad_len + max_len + 2;
 	}
 
 	return ad_len;
-- 
2.7.4

[PATCH 2/3] Bluetooth: Fix local name validation

[Michał Narajowski]

Short name should be shorter than complete name
and it should be a prefix of complete name.

Signed-off-by: Michał Narajowski <michal.narajowski@codecoup.pl>
---
 net/bluetooth/mgmt.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 7b2bac4..5709377 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -3099,6 +3099,24 @@ unlock:
 	hci_dev_unlock(hdev);
 }
 
+static bool name_is_valid(u8 *complete_name, u8 *short_name)
+{
+	size_t complete_len = strlen(complete_name);
+	size_t short_len = strlen(short_name);
+
+	if (complete_len <= short_len)
+		return false;
+
+	/* Core Specification Supplement, A, 1.2.1:
+	 * A shortened name shall only contain contiguous characters from
+	 * the beginning of the full name.
+	 */
+	if (strncmp(short_name, complete_name, short_len) != 0)
+		return false;
+
+	return true;
+}
+
 static int set_local_name(struct sock *sk, struct hci_dev *hdev, void *data,
 			  u16 len)
 {
@@ -3122,6 +3140,12 @@ static int set_local_name(struct sock *sk, struct hci_dev *hdev, void *data,
 		goto failed;
 	}
 
+	if (!name_is_valid(cp->name, cp->short_name)) {
+		err = mgmt_cmd_status(sk, hdev->id, MGMT_OP_SET_LOCAL_NAME,
+				      MGMT_STATUS_INVALID_PARAMS);
+		goto failed;
+	}
+
 	memcpy(hdev->short_name, cp->short_name, sizeof(hdev->short_name));
 
 	if (!hdev_is_powered(hdev)) {
-- 
2.7.4

[PATCH 3/3] Bluetooth: Fix not updating scan rsp when adv off

[Michał Narajowski]

Scan response data should not be updated unless there
is an advertising instance.

Signed-off-by: Michał Narajowski <michal.narajowski@codecoup.pl>
---
 net/bluetooth/mgmt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 5709377..8125ab3 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -3181,7 +3181,7 @@ static int set_local_name(struct sock *sk, struct hci_dev *hdev, void *data,
 	/* The name is stored in the scan response data and so
 	 * no need to udpate the advertising data here.
 	 */
-	if (lmp_le_capable(hdev))
+	if (lmp_le_capable(hdev) && hci_dev_test_flag(hdev, HCI_ADVERTISING))
 		__hci_req_update_scan_rsp_data(&req, hdev->cur_adv_instance);
 
 	err = hci_req_run(&req, set_name_complete);
-- 
2.7.4

Re: [PATCH 1/3] Bluetooth: Fix local name in scan rsp

[Marcel Holtmann]

Hi Michal,

> Use complete name if it fits. If not and there is short name
> check if it fits. If not then use shortened name as prefix
> of complete name.
> 
> Signed-off-by: Michał Narajowski <michal.narajowski@codecoup.pl>
> ---
> net/bluetooth/hci_request.c | 46 +++++++++++++++++++++++++++++++++------------
> 1 file changed, 34 insertions(+), 12 deletions(-)
> 
> diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
> index c813568..880758c 100644
> --- a/net/bluetooth/hci_request.c
> +++ b/net/bluetooth/hci_request.c
> @@ -973,25 +973,47 @@ void __hci_req_enable_advertising(struct hci_request *req)
> 
> static u8 append_local_name(struct hci_dev *hdev, u8 *ptr, u8 ad_len)
> {
> -	size_t name_len;
> +	size_t complete_len;
> +	size_t short_len;
> 	int max_len;
> 
> 	max_len = HCI_MAX_AD_LENGTH - ad_len - 2;
> -	name_len = strlen(hdev->dev_name);
> -	if (name_len > 0 && max_len > 0) {
> +	complete_len = strlen(hdev->dev_name);
> +	short_len = strlen(hdev->short_name);
> 
> -		if (name_len > max_len) {
> -			name_len = max_len;
> -			ptr[1] = EIR_NAME_SHORT;
> -		} else
> -			ptr[1] = EIR_NAME_COMPLETE;
> +	/* no space left for name */
> +	if (max_len < 1)
> +		return ad_len;
> 
> -		ptr[0] = name_len + 1;
> +	/* no name set */
> +	if (!complete_len)
> +		return ad_len;
> 
> -		memcpy(ptr + 2, hdev->dev_name, name_len);
> +	/* complete name fits */
> +	if (complete_len <= max_len) {
> +		ptr[0] = complete_len + 1;
> +		ptr[1] = EIR_NAME_COMPLETE;
> +		memcpy(ptr + 2, hdev->dev_name, complete_len);
> 
> -		ad_len += (name_len + 2);
> -		ptr += (name_len + 2);
> +		return ad_len + complete_len + 2;
> +	}

so what we discussed is that at minimum 11 octets of name will be included into the scan response. That is the same size of the Short_name (which includes the nul-byte).

If the full name is 11 octets or smaller, then that is included. If the full name is longer and a short name has been set, then the short name is used. If the short name is not set, then the full name is truncated to 11 octets.

For the case where the full name is 11 octets or smaller, the complete tag is used. For the case where the short name is used or the full name is truncated, the partial tag is used.

> +
> +	/* shortened name set and fits */

Just a note here, it is not called shortened name. It is the short name. We clearly separated the full name vs short name in the mgmt API.

Regards

Marcel

Re: [PATCH 3/3] Bluetooth: Fix not updating scan rsp when adv off

[Marcel Holtmann]

Hi Michal,

> Scan response data should not be updated unless there
> is an advertising instance.
> 
> Signed-off-by: Michał Narajowski <michal.narajowski@codecoup.pl>
> ---
> net/bluetooth/mgmt.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel

Re: [PATCH 2/3] Bluetooth: Fix local name validation

[Marcel Holtmann]

Hi Michal,

> Short name should be shorter than complete name
> and it should be a prefix of complete name.
> 
> Signed-off-by: Michał Narajowski <michal.narajowski@codecoup.pl>
> ---
> net/bluetooth/mgmt.c | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
> 
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 7b2bac4..5709377 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -3099,6 +3099,24 @@ unlock:
> 	hci_dev_unlock(hdev);
> }
> 
> +static bool name_is_valid(u8 *complete_name, u8 *short_name)
> +{
> +	size_t complete_len = strlen(complete_name);
> +	size_t short_len = strlen(short_name);
> +
> +	if (complete_len <= short_len)
> +		return false;
> +
> +	/* Core Specification Supplement, A, 1.2.1:
> +	 * A shortened name shall only contain contiguous characters from
> +	 * the beginning of the full name.
> +	 */
> +	if (strncmp(short_name, complete_name, short_len) != 0)
> +		return false;
> +
> +	return true;
> +}
> +

actually I do not want to enforce that from the kernel side. Lets keep the mgmt interface flexible and let bluetoothd enforce this if needed.

Regards

Marcel

Re: [PATCH 1/3] Bluetooth: Fix local name in scan rsp

[Szymon Janc]

Hi Marcel,

On Thursday, 22 September 2016 17:47:15 CEST Marcel Holtmann wrote:
> Hi Michal,
>=20
> > Use complete name if it fits. If not and there is short name
> > check if it fits. If not then use shortened name as prefix
> > of complete name.
> >=20
> > Signed-off-by: Micha=C5=82 Narajowski <michal.narajowski@codecoup.pl>
> > ---
> > net/bluetooth/hci_request.c | 46
> > +++++++++++++++++++++++++++++++++------------ 1 file changed, 34
> > insertions(+), 12 deletions(-)
> >=20
> > diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
> > index c813568..880758c 100644
> > --- a/net/bluetooth/hci_request.c
> > +++ b/net/bluetooth/hci_request.c
> > @@ -973,25 +973,47 @@ void __hci_req_enable_advertising(struct hci_requ=
est
> > *req)
> >=20
> > static u8 append_local_name(struct hci_dev *hdev, u8 *ptr, u8 ad_len)
> > {
> > -	size_t name_len;
> > +	size_t complete_len;
> > +	size_t short_len;
> >=20
> > 	int max_len;
> > =09
> > 	max_len =3D HCI_MAX_AD_LENGTH - ad_len - 2;
> >=20
> > -	name_len =3D strlen(hdev->dev_name);
> > -	if (name_len > 0 && max_len > 0) {
> > +	complete_len =3D strlen(hdev->dev_name);
> > +	short_len =3D strlen(hdev->short_name);
> >=20
> > -		if (name_len > max_len) {
> > -			name_len =3D max_len;
> > -			ptr[1] =3D EIR_NAME_SHORT;
> > -		} else
> > -			ptr[1] =3D EIR_NAME_COMPLETE;
> > +	/* no space left for name */
> > +	if (max_len < 1)
> > +		return ad_len;
> >=20
> > -		ptr[0] =3D name_len + 1;
> > +	/* no name set */
> > +	if (!complete_len)
> > +		return ad_len;
> >=20
> > -		memcpy(ptr + 2, hdev->dev_name, name_len);
> > +	/* complete name fits */
> > +	if (complete_len <=3D max_len) {
> > +		ptr[0] =3D complete_len + 1;
> > +		ptr[1] =3D EIR_NAME_COMPLETE;
> > +		memcpy(ptr + 2, hdev->dev_name, complete_len);
> >=20
> > -		ad_len +=3D (name_len + 2);
> > -		ptr +=3D (name_len + 2);
> > +		return ad_len + complete_len + 2;
> > +	}
>=20
> so what we discussed is that at minimum 11 octets of name will be included
> into the scan response. That is the same size of the Short_name (which
> includes the nul-byte).

minimum? And do we need to put null byte there?

>=20
> If the full name is 11 octets or smaller, then that is included. If the f=
ull
> name is longer and a short name has been set, then the short name is used.
> If the short name is not set, then the full name is truncated to 11 octet=
s.
>=20
> For the case where the full name is 11 octets or smaller, the complete tag
> is used. For the case where the short name is used or the full name is
> truncated, the partial tag is used.

So we never include full name if it is longer then 11 octets (with null)?
BTW where this 11 octets value came from? I cannot find such requirement in=
=20
spec. Or this is just limited on mgmt interface level?

I don't undertand why not to include complete name if it fits, even if long=
er=20
than 11 octets.

> > +
> > +	/* shortened name set and fits */
>=20
> Just a note here, it is not called shortened name. It is the short name. =
We
> clearly separated the full name vs short name in the mgmt API.

It is called 'shortened' in both CoreSpec and CSS.=20

> Regards
>=20
> Marcel
>=20
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth"
> in the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


=2D-=20
pozdrawiam
Szymon Janc

Re: [PATCH 1/3] Bluetooth: Fix local name in scan rsp

[Marcel Holtmann]

Hi Szymon,

>>> Use complete name if it fits. If not and there is short name
>>> check if it fits. If not then use shortened name as prefix
>>> of complete name.
>>> 
>>> Signed-off-by: Michał Narajowski <michal.narajowski@codecoup.pl>
>>> ---
>>> net/bluetooth/hci_request.c | 46
>>> +++++++++++++++++++++++++++++++++------------ 1 file changed, 34
>>> insertions(+), 12 deletions(-)
>>> 
>>> diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
>>> index c813568..880758c 100644
>>> --- a/net/bluetooth/hci_request.c
>>> +++ b/net/bluetooth/hci_request.c
>>> @@ -973,25 +973,47 @@ void __hci_req_enable_advertising(struct hci_request
>>> *req)
>>> 
>>> static u8 append_local_name(struct hci_dev *hdev, u8 *ptr, u8 ad_len)
>>> {
>>> -	size_t name_len;
>>> +	size_t complete_len;
>>> +	size_t short_len;
>>> 
>>> 	int max_len;
>>> 	
>>> 	max_len = HCI_MAX_AD_LENGTH - ad_len - 2;
>>> 
>>> -	name_len = strlen(hdev->dev_name);
>>> -	if (name_len > 0 && max_len > 0) {
>>> +	complete_len = strlen(hdev->dev_name);
>>> +	short_len = strlen(hdev->short_name);
>>> 
>>> -		if (name_len > max_len) {
>>> -			name_len = max_len;
>>> -			ptr[1] = EIR_NAME_SHORT;
>>> -		} else
>>> -			ptr[1] = EIR_NAME_COMPLETE;
>>> +	/* no space left for name */
>>> +	if (max_len < 1)
>>> +		return ad_len;
>>> 
>>> -		ptr[0] = name_len + 1;
>>> +	/* no name set */
>>> +	if (!complete_len)
>>> +		return ad_len;
>>> 
>>> -		memcpy(ptr + 2, hdev->dev_name, name_len);
>>> +	/* complete name fits */
>>> +	if (complete_len <= max_len) {
>>> +		ptr[0] = complete_len + 1;
>>> +		ptr[1] = EIR_NAME_COMPLETE;
>>> +		memcpy(ptr + 2, hdev->dev_name, complete_len);
>>> 
>>> -		ad_len += (name_len + 2);
>>> -		ptr += (name_len + 2);
>>> +		return ad_len + complete_len + 2;
>>> +	}
>> 
>> so what we discussed is that at minimum 11 octets of name will be included
>> into the scan response. That is the same size of the Short_name (which
>> includes the nul-byte).
> 
> minimum? And do we need to put null byte there?

I think the term maximum would have been better. 1-11 octets of name should be included. For the nul-byte, we need to double check the spec., but I think it is required to be included. If not, then we can leave it out.

>> If the full name is 11 octets or smaller, then that is included. If the full
>> name is longer and a short name has been set, then the short name is used.
>> If the short name is not set, then the full name is truncated to 11 octets.
>> 
>> For the case where the full name is 11 octets or smaller, the complete tag
>> is used. For the case where the short name is used or the full name is
>> truncated, the partial tag is used.
> 
> So we never include full name if it is longer then 11 octets (with null)?
> BTW where this 11 octets value came from? I cannot find such requirement in 
> spec. Or this is just limited on mgmt interface level?

Until we have longer advertising packets, I think the 11 octets sound like a good compromise. Userspace can always decide to drive scan response by itself. Or do you have a better idea for a policy?

The 11 octets is a mgmt limit for the short name. We decided on that when inventing mgmt interface.

> I don't undertand why not to include complete name if it fits, even if longer 
> than 11 octets.

Feel free to define the policy then. It gets really complicated if userspace wants to include appearance, name and then its own data. If the name occupies the rest. Also when to decide when to use the short name vs long name. If we limit at the short name size, then the policy is simple.

>>> +
>>> +	/* shortened name set and fits */
>> 
>> Just a note here, it is not called shortened name. It is the short name. We
>> clearly separated the full name vs short name in the mgmt API.
> 
> It is called 'shortened' in both CoreSpec and CSS. 

And short name in mgmt.

Regards

Marcel