* [PATCH v1] hog: Check security level before setting @ 2024-11-18 9:49 Jiayang Mao 2024-11-18 11:12 ` [v1] " bluez.test.bot 2024-11-18 15:13 ` [PATCH v1] " Luiz Augusto von Dentz 0 siblings, 2 replies; 6+ messages in thread From: Jiayang Mao @ 2024-11-18 9:49 UTC (permalink / raw) To: linux-bluetooth; +Cc: quic_chejiang bt_gatt_client_set_security could fail if the security level is already BT_ATT_SECURITY_MEDIUM. So, get and check the security level before setting it. Signed-off-by: Jiayang Mao <quic_jiaymao@quicinc.com> --- profiles/input/hog.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/profiles/input/hog.c b/profiles/input/hog.c index 017e320f0..011cc0a88 100644 --- a/profiles/input/hog.c +++ b/profiles/input/hog.c @@ -191,8 +191,10 @@ static int hog_accept(struct btd_service *service) return -ECONNREFUSED; client = btd_device_get_gatt_client(device); - if (!bt_gatt_client_set_security(client, - BT_ATT_SECURITY_MEDIUM)) + if (BT_ATT_SECURITY_MEDIUM != + bt_gatt_client_get_security(client) && + !bt_gatt_client_set_security(client, + BT_ATT_SECURITY_MEDIUM)) return -ECONNREFUSED; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* RE: [v1] hog: Check security level before setting 2024-11-18 9:49 [PATCH v1] hog: Check security level before setting Jiayang Mao @ 2024-11-18 11:12 ` bluez.test.bot 2024-11-18 15:13 ` [PATCH v1] " Luiz Augusto von Dentz 1 sibling, 0 replies; 6+ messages in thread From: bluez.test.bot @ 2024-11-18 11:12 UTC (permalink / raw) To: linux-bluetooth, quic_jiaymao [-- Attachment #1: Type: text/plain, Size: 1260 bytes --] This is automated email and please do not reply to this email! Dear submitter, Thank you for submitting the patches to the linux bluetooth mailing list. This is a CI test results with your patch series: PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=910561 ---Test result--- Test Summary: CheckPatch PENDING 0.23 seconds GitLint PENDING 0.18 seconds BuildEll PASS 20.45 seconds BluezMake PASS 1523.29 seconds MakeCheck PASS 13.00 seconds MakeDistcheck PASS 159.15 seconds CheckValgrind PASS 213.41 seconds CheckSmatch PASS 272.24 seconds bluezmakeextell PASS 98.55 seconds IncrementalBuild PENDING 0.29 seconds ScanBuild PASS 839.87 seconds Details ############################## Test: CheckPatch - PENDING Desc: Run checkpatch.pl script Output: ############################## Test: GitLint - PENDING Desc: Run gitlint Output: ############################## Test: IncrementalBuild - PENDING Desc: Incremental build with the patches in the series Output: --- Regards, Linux Bluetooth ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1] hog: Check security level before setting 2024-11-18 9:49 [PATCH v1] hog: Check security level before setting Jiayang Mao 2024-11-18 11:12 ` [v1] " bluez.test.bot @ 2024-11-18 15:13 ` Luiz Augusto von Dentz 2024-11-19 3:22 ` Jiayang Mao 1 sibling, 1 reply; 6+ messages in thread From: Luiz Augusto von Dentz @ 2024-11-18 15:13 UTC (permalink / raw) To: Jiayang Mao; +Cc: linux-bluetooth, quic_chejiang Hi Jiayang, On Mon, Nov 18, 2024 at 4:49 AM Jiayang Mao <quic_jiaymao@quicinc.com> wrote: > > bt_gatt_client_set_security could fail if the security level is > already BT_ATT_SECURITY_MEDIUM. So, get and check the security > level before setting it. Seems a bit strange that this is not handled by the kernel, can you elaborate on the conditions to trigger it? > Signed-off-by: Jiayang Mao <quic_jiaymao@quicinc.com> > --- > profiles/input/hog.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/profiles/input/hog.c b/profiles/input/hog.c > index 017e320f0..011cc0a88 100644 > --- a/profiles/input/hog.c > +++ b/profiles/input/hog.c > @@ -191,8 +191,10 @@ static int hog_accept(struct btd_service *service) > return -ECONNREFUSED; > > client = btd_device_get_gatt_client(device); > - if (!bt_gatt_client_set_security(client, > - BT_ATT_SECURITY_MEDIUM)) > + if (BT_ATT_SECURITY_MEDIUM != > + bt_gatt_client_get_security(client) && > + !bt_gatt_client_set_security(client, > + BT_ATT_SECURITY_MEDIUM)) > return -ECONNREFUSED; Definitely not the right way to fix this since there might be other places that do attempt to set the security, so Id got with something like the following: diff --git a/src/shared/att.c b/src/shared/att.c index 4a406f4b91a4..dabbdb4315eb 100644 --- a/src/shared/att.c +++ b/src/shared/att.c @@ -727,6 +727,9 @@ static bool bt_att_chan_set_security(struct bt_att_chan *chan, int level) { struct bt_security sec; + if (level == bt_att_chan_get_security(chan)) + return true; + if (chan->type == BT_ATT_LOCAL) { chan->sec_level = level; return true; > } > > -- > 2.25.1 > > -- Luiz Augusto von Dentz ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v1] hog: Check security level before setting 2024-11-18 15:13 ` [PATCH v1] " Luiz Augusto von Dentz @ 2024-11-19 3:22 ` Jiayang Mao 2024-11-19 16:23 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 6+ messages in thread From: Jiayang Mao @ 2024-11-19 3:22 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: linux-bluetooth, quic_chejiang, Jiayang Mao (QUIC) Hi Luiz, On 2024/11/18 23:13, Luiz Augusto von Dentz wrote: > Hi Jiayang, > > On Mon, Nov 18, 2024 at 4:49 AM Jiayang Mao <quic_jiaymao@quicinc.com> wrote: >> >> bt_gatt_client_set_security could fail if the security level is >> already BT_ATT_SECURITY_MEDIUM. So, get and check the security >> level before setting it. > > Seems a bit strange that this is not handled by the kernel, can you > elaborate on the conditions to trigger it? > In the kernel, the failure happens when smp_sufficient_security() in 'net/bluetooth/smp.c' returns true. In some cases, when security level is already MEDIUM but long term key is not ready, setting security level will fail. Checking security level before setting it can prevent this failure. >> Signed-off-by: Jiayang Mao <quic_jiaymao@quicinc.com> >> --- >> profiles/input/hog.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/profiles/input/hog.c b/profiles/input/hog.c >> index 017e320f0..011cc0a88 100644 >> --- a/profiles/input/hog.c >> +++ b/profiles/input/hog.c >> @@ -191,8 +191,10 @@ static int hog_accept(struct btd_service *service) >> return -ECONNREFUSED; >> >> client = btd_device_get_gatt_client(device); >> - if (!bt_gatt_client_set_security(client, >> - BT_ATT_SECURITY_MEDIUM)) >> + if (BT_ATT_SECURITY_MEDIUM != >> + bt_gatt_client_get_security(client) && >> + !bt_gatt_client_set_security(client, >> + BT_ATT_SECURITY_MEDIUM)) >> return -ECONNREFUSED; > > > Definitely not the right way to fix this since there might be other > places that do attempt to set the security, so Id got with something > like the following: > > diff --git a/src/shared/att.c b/src/shared/att.c > index 4a406f4b91a4..dabbdb4315eb 100644 > --- a/src/shared/att.c > +++ b/src/shared/att.c > @@ -727,6 +727,9 @@ static bool bt_att_chan_set_security(struct > bt_att_chan *chan, int level) > { > struct bt_security sec; > > + if (level == bt_att_chan_get_security(chan)) > + return true; > + > if (chan->type == BT_ATT_LOCAL) { > chan->sec_level = level; > return true; > >> } >> >> -- >> 2.25.1 >> >> > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1] hog: Check security level before setting 2024-11-19 3:22 ` Jiayang Mao @ 2024-11-19 16:23 ` Luiz Augusto von Dentz 2024-11-20 8:12 ` Jiayang Mao 0 siblings, 1 reply; 6+ messages in thread From: Luiz Augusto von Dentz @ 2024-11-19 16:23 UTC (permalink / raw) To: Jiayang Mao; +Cc: linux-bluetooth, quic_chejiang, Jiayang Mao (QUIC) Hi Jiayang, On Mon, Nov 18, 2024 at 10:23 PM Jiayang Mao <quic_jiaymao@quicinc.com> wrote: > > Hi Luiz, > > On 2024/11/18 23:13, Luiz Augusto von Dentz wrote: > > Hi Jiayang, > > > > On Mon, Nov 18, 2024 at 4:49 AM Jiayang Mao <quic_jiaymao@quicinc.com> wrote: > >> > >> bt_gatt_client_set_security could fail if the security level is > >> already BT_ATT_SECURITY_MEDIUM. So, get and check the security > >> level before setting it. > > > > Seems a bit strange that this is not handled by the kernel, can you > > elaborate on the conditions to trigger it? > > > > In the kernel, the failure happens when smp_sufficient_security() in > 'net/bluetooth/smp.c' returns true. In some cases, when security level > is already MEDIUM but long term key is not ready, setting security level > will fail. Checking security level before setting it can prevent this > failure. Hmm, is this about encryption change happening before the LTK is distributed? Ive seen that sometimes remote devices also have this problem e.g. GATT read return encryption required error but the link is already encrypted. Btw, I assume the problem is in this following code: https://github.com/torvalds/linux/blob/master/net/bluetooth/l2cap_sock.c#L931 That would fail with 1 because smp_sufficient_security would return true but we assume that to be an error, well it is an error but I guess it should have been -EALREADY that way one does not need to check the error again: diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c index 18e89e764f3b..ada0915c7421 100644 --- a/net/bluetooth/l2cap_sock.c +++ b/net/bluetooth/l2cap_sock.c @@ -919,6 +919,11 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname, break; } + if (chan->sec_level == sec.level) { + err = -EALREADY; + break; + } + chan->sec_level = sec.level; if (!chan->conn) Anyway I think it is safer to do that on userspace first so it acts properly with older kernels. > >> Signed-off-by: Jiayang Mao <quic_jiaymao@quicinc.com> > >> --- > >> profiles/input/hog.c | 6 ++++-- > >> 1 file changed, 4 insertions(+), 2 deletions(-) > >> > >> diff --git a/profiles/input/hog.c b/profiles/input/hog.c > >> index 017e320f0..011cc0a88 100644 > >> --- a/profiles/input/hog.c > >> +++ b/profiles/input/hog.c > >> @@ -191,8 +191,10 @@ static int hog_accept(struct btd_service *service) > >> return -ECONNREFUSED; > >> > >> client = btd_device_get_gatt_client(device); > >> - if (!bt_gatt_client_set_security(client, > >> - BT_ATT_SECURITY_MEDIUM)) > >> + if (BT_ATT_SECURITY_MEDIUM != > >> + bt_gatt_client_get_security(client) && > >> + !bt_gatt_client_set_security(client, > >> + BT_ATT_SECURITY_MEDIUM)) > >> return -ECONNREFUSED; > > > > > > Definitely not the right way to fix this since there might be other > > places that do attempt to set the security, so Id got with something > > like the following: > > > > diff --git a/src/shared/att.c b/src/shared/att.c > > index 4a406f4b91a4..dabbdb4315eb 100644 > > --- a/src/shared/att.c > > +++ b/src/shared/att.c > > @@ -727,6 +727,9 @@ static bool bt_att_chan_set_security(struct > > bt_att_chan *chan, int level) > > { > > struct bt_security sec; > > > > + if (level == bt_att_chan_get_security(chan)) > > + return true; > > + > > if (chan->type == BT_ATT_LOCAL) { > > chan->sec_level = level; > > return true; > > > >> } > >> > >> -- > >> 2.25.1 > >> > >> > > > > > -- Luiz Augusto von Dentz ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v1] hog: Check security level before setting 2024-11-19 16:23 ` Luiz Augusto von Dentz @ 2024-11-20 8:12 ` Jiayang Mao 0 siblings, 0 replies; 6+ messages in thread From: Jiayang Mao @ 2024-11-20 8:12 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: linux-bluetooth, quic_chejiang, Jiayang Mao (QUIC) Hi Luiz, On 2024/11/20 0:23, Luiz Augusto von Dentz wrote: > Hi Jiayang, > > On Mon, Nov 18, 2024 at 10:23 PM Jiayang Mao <quic_jiaymao@quicinc.com> wrote: >> >> Hi Luiz, >> >> On 2024/11/18 23:13, Luiz Augusto von Dentz wrote: >>> Hi Jiayang, >>> >>> On Mon, Nov 18, 2024 at 4:49 AM Jiayang Mao <quic_jiaymao@quicinc.com> wrote: >>>> >>>> bt_gatt_client_set_security could fail if the security level is >>>> already BT_ATT_SECURITY_MEDIUM. So, get and check the security >>>> level before setting it. >>> >>> Seems a bit strange that this is not handled by the kernel, can you >>> elaborate on the conditions to trigger it? >>> >> >> In the kernel, the failure happens when smp_sufficient_security() in >> 'net/bluetooth/smp.c' returns true. In some cases, when security level >> is already MEDIUM but long term key is not ready, setting security level >> will fail. Checking security level before setting it can prevent this >> failure. > > Hmm, is this about encryption change happening before the LTK is > distributed? Ive seen that sometimes remote devices also have this > problem e.g. GATT read return encryption required error but the link > is already encrypted. > > Btw, I assume the problem is in this following code: > > https://github.com/torvalds/linux/blob/master/net/bluetooth/l2cap_sock.c#L931 > > That would fail with 1 because smp_sufficient_security would return > true but we assume that to be an error, well it is an error but I > guess it should have been -EALREADY that way one does not need to > check the error again: > > diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c > index 18e89e764f3b..ada0915c7421 100644 > --- a/net/bluetooth/l2cap_sock.c > +++ b/net/bluetooth/l2cap_sock.c > @@ -919,6 +919,11 @@ static int l2cap_sock_setsockopt(struct socket > *sock, int level, int optname, > break; > } > > + if (chan->sec_level == sec.level) { > + err = -EALREADY; > + break; > + } > + > chan->sec_level = sec.level; > > if (!chan->conn) > > Anyway I think it is safer to do that on userspace first so it acts > properly with older kernels. > Thanks. Following your previous suggestion, I updated another patch to move the change to att.c: [v1] att: Check security level before setting >>>> Signed-off-by: Jiayang Mao <quic_jiaymao@quicinc.com> >>>> --- >>>> profiles/input/hog.c | 6 ++++-- >>>> 1 file changed, 4 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/profiles/input/hog.c b/profiles/input/hog.c >>>> index 017e320f0..011cc0a88 100644 >>>> --- a/profiles/input/hog.c >>>> +++ b/profiles/input/hog.c >>>> @@ -191,8 +191,10 @@ static int hog_accept(struct btd_service *service) >>>> return -ECONNREFUSED; >>>> >>>> client = btd_device_get_gatt_client(device); >>>> - if (!bt_gatt_client_set_security(client, >>>> - BT_ATT_SECURITY_MEDIUM)) >>>> + if (BT_ATT_SECURITY_MEDIUM != >>>> + bt_gatt_client_get_security(client) && >>>> + !bt_gatt_client_set_security(client, >>>> + BT_ATT_SECURITY_MEDIUM)) >>>> return -ECONNREFUSED; >>> >>> >>> Definitely not the right way to fix this since there might be other >>> places that do attempt to set the security, so Id got with something >>> like the following: >>> >>> diff --git a/src/shared/att.c b/src/shared/att.c >>> index 4a406f4b91a4..dabbdb4315eb 100644 >>> --- a/src/shared/att.c >>> +++ b/src/shared/att.c >>> @@ -727,6 +727,9 @@ static bool bt_att_chan_set_security(struct >>> bt_att_chan *chan, int level) >>> { >>> struct bt_security sec; >>> >>> + if (level == bt_att_chan_get_security(chan)) >>> + return true; >>> + >>> if (chan->type == BT_ATT_LOCAL) { >>> chan->sec_level = level; >>> return true; >>> >>>> } >>>> >>>> -- >>>> 2.25.1 >>>> >>>> >>> >>> >> > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-11-20 8:12 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-11-18 9:49 [PATCH v1] hog: Check security level before setting Jiayang Mao 2024-11-18 11:12 ` [v1] " bluez.test.bot 2024-11-18 15:13 ` [PATCH v1] " Luiz Augusto von Dentz 2024-11-19 3:22 ` Jiayang Mao 2024-11-19 16:23 ` Luiz Augusto von Dentz 2024-11-20 8:12 ` Jiayang Mao
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.