[PATCH] Bluetooth: SCO: check for codecs->num_codecs == 1 before assigning to sco_pi(sk)->codec

[PATCH] Bluetooth: SCO: check for codecs->num_codecs == 1 before assigning to sco_pi(sk)->codec

[Stefan Metzmacher]

copy_struct_from_sockptr() fill 'buffer' in
sco_sock_setsockopt() with zeros, so there's no
real problem.

But it actually looks strange to do this,
without checking all of codecs->codecs[0]
really comes from userspace:

  sco_pi(sk)->codec = codecs->codecs[0];

As only optlen < sizeof(struct bt_codecs) is checked
and codecs->num_codecs is not checked against != 1,
but only <= 1, and the space for the additional struct bt_codec
is not checked.

Note I don't understand bluetooth and I didn't do any runtime
tests with this! I just found it when debugging a problem
in copy_struct_from_sockptr().

I just added this to check the size is as expected:

  BUILD_BUG_ON(struct_size(codecs, codecs, 0) != 1);
  BUILD_BUG_ON(struct_size(codecs, codecs, 1) != 8);

And made sure it still compiles using this:

  make CF=-D__CHECK_ENDIAN__ W=1ce C=1 net/bluetooth/sco.o

Fixes: 3e643e4efa1e ("Bluetooth: Improve setsockopt() handling of malformed user input")
Cc: Michal Luczaj <mhal@rbox.co>
Cc: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
Cc: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: David Wei <dw@davidwei.uk>
Cc: linux-bluetooth@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Stefan Metzmacher <metze@samba.org>
---
 net/bluetooth/sco.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index b84587811ef4..359eabf7dddb 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -1045,7 +1045,13 @@ static int sco_sock_setsockopt(struct socket *sock, int level, int optname,
 
 		codecs = (void *)buffer;
 
-		if (codecs->num_codecs > 1) {
+		if (codecs->num_codecs != 1) {
+			hci_dev_put(hdev);
+			err = -EINVAL;
+			break;
+		}
+
+		if (optlen < struct_size(codecs, codecs, codecs->num_codecs)) {
 			hci_dev_put(hdev);
 			err = -EINVAL;
 			break;
-- 
2.43.0

RE: Bluetooth: SCO: check for codecs->num_codecs == 1 before assigning to sco_pi(sk)->codec

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2593 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=1078240

---Test result---

Test Summary:
CheckPatch                    PENDING   0.38 seconds
GitLint                       PENDING   0.29 seconds
SubjectPrefix                 PASS      0.44 seconds
BuildKernel                   PASS      24.36 seconds
CheckAllWarning               PASS      26.63 seconds
CheckSparse                   PASS      25.66 seconds
BuildKernel32                 PASS      23.82 seconds
TestRunnerSetup               PASS      514.18 seconds
TestRunner_l2cap-tester       PASS      27.33 seconds
TestRunner_iso-tester         PASS      38.62 seconds
TestRunner_bnep-tester        PASS      6.27 seconds
TestRunner_mgmt-tester        FAIL      111.68 seconds
TestRunner_rfcomm-tester      PASS      9.25 seconds
TestRunner_sco-tester         FAIL      14.31 seconds
TestRunner_ioctl-tester       PASS      10.02 seconds
TestRunner_mesh-tester        FAIL      11.61 seconds
TestRunner_smp-tester         PASS      8.56 seconds
TestRunner_userchan-tester    PASS      6.63 seconds
IncrementalBuild              PENDING   0.53 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 494, Passed: 489 (99.0%), Failed: 1, Not Run: 4

Failed Test Cases
Read Exp Feature - Success                           Failed       0.104 seconds
##############################
Test: TestRunner_sco-tester - FAIL
Desc: Run sco-tester with test-runner
Output:
WARNING: possible circular locking dependency detected
BUG: sleeping function called from invalid context at net/core/sock.c:3782
Total: 30, Passed: 30 (100.0%), Failed: 0, Not Run: 0
##############################
Test: TestRunner_mesh-tester - FAIL
Desc: Run mesh-tester with test-runner
Output:
Total: 10, Passed: 8 (80.0%), Failed: 2, Not Run: 0

Failed Test Cases
Mesh - Send cancel - 1                               Timed out    1.969 seconds
Mesh - Send cancel - 2                               Timed out    1.995 seconds
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth

Re: [PATCH] Bluetooth: SCO: check for codecs->num_codecs == 1 before assigning to sco_pi(sk)->codec

[Luiz Augusto von Dentz]

Hi Stefan,

On Tue, Apr 7, 2026 at 11:13 AM Stefan Metzmacher <metze@samba.org> wrote:
>
> copy_struct_from_sockptr() fill 'buffer' in
> sco_sock_setsockopt() with zeros, so there's no
> real problem.
>
> But it actually looks strange to do this,
> without checking all of codecs->codecs[0]
> really comes from userspace:
>
>   sco_pi(sk)->codec = codecs->codecs[0];
>
> As only optlen < sizeof(struct bt_codecs) is checked
> and codecs->num_codecs is not checked against != 1,
> but only <= 1, and the space for the additional struct bt_codec
> is not checked.
>
> Note I don't understand bluetooth and I didn't do any runtime
> tests with this! I just found it when debugging a problem
> in copy_struct_from_sockptr().
>
> I just added this to check the size is as expected:
>
>   BUILD_BUG_ON(struct_size(codecs, codecs, 0) != 1);
>   BUILD_BUG_ON(struct_size(codecs, codecs, 1) != 8);
>
> And made sure it still compiles using this:
>
>   make CF=-D__CHECK_ENDIAN__ W=1ce C=1 net/bluetooth/sco.o
>
> Fixes: 3e643e4efa1e ("Bluetooth: Improve setsockopt() handling of malformed user input")
> Cc: Michal Luczaj <mhal@rbox.co>
> Cc: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> Cc: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
> Cc: Marcel Holtmann <marcel@holtmann.org>
> Cc: David Wei <dw@davidwei.uk>
> Cc: linux-bluetooth@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Stefan Metzmacher <metze@samba.org>
> ---
>  net/bluetooth/sco.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index b84587811ef4..359eabf7dddb 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -1045,7 +1045,13 @@ static int sco_sock_setsockopt(struct socket *sock, int level, int optname,
>
>                 codecs = (void *)buffer;
>
> -               if (codecs->num_codecs > 1) {
> +               if (codecs->num_codecs != 1) {
> +                       hci_dev_put(hdev);
> +                       err = -EINVAL;
> +                       break;
> +               }
> +
> +               if (optlen < struct_size(codecs, codecs, codecs->num_codecs)) {

I guess this is odd because there was no need for this code to exist,
BT_CODEC should have used struct bt_codec rather than struct bt_codecs
since we can only set exactly one codec, that said I don't think we
can change this now thus why we need to perform these checks, anyway
back to your changes why don't we have both checks for num_codecs != 1
&& optlen < struct_size(codecs, codecs, codecs->num_codecs)) since the
handling is the same?

>                         hci_dev_put(hdev);
>                         err = -EINVAL;
>                         break;
> --
> 2.43.0
>


-- 
Luiz Augusto von Dentz

Re: [PATCH] Bluetooth: SCO: check for codecs->num_codecs == 1 before assigning to sco_pi(sk)->codec

[Stefan Metzmacher]

Hi Luiz,

> On Tue, Apr 7, 2026 at 11:13 AM Stefan Metzmacher <metze@samba.org> wrote:
>>
>> copy_struct_from_sockptr() fill 'buffer' in
>> sco_sock_setsockopt() with zeros, so there's no
>> real problem.
>>
>> But it actually looks strange to do this,
>> without checking all of codecs->codecs[0]
>> really comes from userspace:
>>
>>    sco_pi(sk)->codec = codecs->codecs[0];
>>
>> As only optlen < sizeof(struct bt_codecs) is checked
>> and codecs->num_codecs is not checked against != 1,
>> but only <= 1, and the space for the additional struct bt_codec
>> is not checked.
>>
>> Note I don't understand bluetooth and I didn't do any runtime
>> tests with this! I just found it when debugging a problem
>> in copy_struct_from_sockptr().
>>
>> I just added this to check the size is as expected:
>>
>>    BUILD_BUG_ON(struct_size(codecs, codecs, 0) != 1);
>>    BUILD_BUG_ON(struct_size(codecs, codecs, 1) != 8);
>>
>> And made sure it still compiles using this:
>>
>>    make CF=-D__CHECK_ENDIAN__ W=1ce C=1 net/bluetooth/sco.o
>>
>> Fixes: 3e643e4efa1e ("Bluetooth: Improve setsockopt() handling of malformed user input")
>> Cc: Michal Luczaj <mhal@rbox.co>
>> Cc: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>> Cc: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
>> Cc: Marcel Holtmann <marcel@holtmann.org>
>> Cc: David Wei <dw@davidwei.uk>
>> Cc: linux-bluetooth@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Stefan Metzmacher <metze@samba.org>
>> ---
>>   net/bluetooth/sco.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
>> index b84587811ef4..359eabf7dddb 100644
>> --- a/net/bluetooth/sco.c
>> +++ b/net/bluetooth/sco.c
>> @@ -1045,7 +1045,13 @@ static int sco_sock_setsockopt(struct socket *sock, int level, int optname,
>>
>>                  codecs = (void *)buffer;
>>
>> -               if (codecs->num_codecs > 1) {
>> +               if (codecs->num_codecs != 1) {
>> +                       hci_dev_put(hdev);
>> +                       err = -EINVAL;
>> +                       break;
>> +               }
>> +
>> +               if (optlen < struct_size(codecs, codecs, codecs->num_codecs)) {
> 
> I guess this is odd because there was no need for this code to exist,
> BT_CODEC should have used struct bt_codec rather than struct bt_codecs
> since we can only set exactly one codec, that said I don't think we
> can change this now thus why we need to perform these checks, anyway
> back to your changes why don't we have both checks for num_codecs != 1
> && optlen < struct_size(codecs, codecs, codecs->num_codecs)) since the
> handling is the same?

Should I submit a v2 using this:
if (codecs->num_codecs != 1 || optlen < struct_size(codecs, codecs, codecs->num_codecs))

Or can you to take it from here?
As I don't really care and the patch is not strictly needed.

Thanks!
metze

Re: [PATCH] Bluetooth: SCO: check for codecs->num_codecs == 1 before assigning to sco_pi(sk)->codec

[Luiz Augusto von Dentz]

Hi Stefan,

On Tue, Apr 7, 2026 at 12:45 PM Stefan Metzmacher <metze@samba.org> wrote:
>
> Hi Luiz,
>
> > On Tue, Apr 7, 2026 at 11:13 AM Stefan Metzmacher <metze@samba.org> wrote:
> >>
> >> copy_struct_from_sockptr() fill 'buffer' in
> >> sco_sock_setsockopt() with zeros, so there's no
> >> real problem.
> >>
> >> But it actually looks strange to do this,
> >> without checking all of codecs->codecs[0]
> >> really comes from userspace:
> >>
> >>    sco_pi(sk)->codec = codecs->codecs[0];
> >>
> >> As only optlen < sizeof(struct bt_codecs) is checked
> >> and codecs->num_codecs is not checked against != 1,
> >> but only <= 1, and the space for the additional struct bt_codec
> >> is not checked.
> >>
> >> Note I don't understand bluetooth and I didn't do any runtime
> >> tests with this! I just found it when debugging a problem
> >> in copy_struct_from_sockptr().
> >>
> >> I just added this to check the size is as expected:
> >>
> >>    BUILD_BUG_ON(struct_size(codecs, codecs, 0) != 1);
> >>    BUILD_BUG_ON(struct_size(codecs, codecs, 1) != 8);
> >>
> >> And made sure it still compiles using this:
> >>
> >>    make CF=-D__CHECK_ENDIAN__ W=1ce C=1 net/bluetooth/sco.o
> >>
> >> Fixes: 3e643e4efa1e ("Bluetooth: Improve setsockopt() handling of malformed user input")
> >> Cc: Michal Luczaj <mhal@rbox.co>
> >> Cc: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> >> Cc: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
> >> Cc: Marcel Holtmann <marcel@holtmann.org>
> >> Cc: David Wei <dw@davidwei.uk>
> >> Cc: linux-bluetooth@vger.kernel.org
> >> Cc: linux-kernel@vger.kernel.org
> >> Signed-off-by: Stefan Metzmacher <metze@samba.org>
> >> ---
> >>   net/bluetooth/sco.c | 8 +++++++-
> >>   1 file changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> >> index b84587811ef4..359eabf7dddb 100644
> >> --- a/net/bluetooth/sco.c
> >> +++ b/net/bluetooth/sco.c
> >> @@ -1045,7 +1045,13 @@ static int sco_sock_setsockopt(struct socket *sock, int level, int optname,
> >>
> >>                  codecs = (void *)buffer;
> >>
> >> -               if (codecs->num_codecs > 1) {
> >> +               if (codecs->num_codecs != 1) {
> >> +                       hci_dev_put(hdev);
> >> +                       err = -EINVAL;
> >> +                       break;
> >> +               }
> >> +
> >> +               if (optlen < struct_size(codecs, codecs, codecs->num_codecs)) {
> >
> > I guess this is odd because there was no need for this code to exist,
> > BT_CODEC should have used struct bt_codec rather than struct bt_codecs
> > since we can only set exactly one codec, that said I don't think we
> > can change this now thus why we need to perform these checks, anyway
> > back to your changes why don't we have both checks for num_codecs != 1
> > && optlen < struct_size(codecs, codecs, codecs->num_codecs)) since the
> > handling is the same?
>
> Should I submit a v2 using this:
> if (codecs->num_codecs != 1 || optlen < struct_size(codecs, codecs, codecs->num_codecs))
>
> Or can you to take it from here?
> As I don't really care and the patch is not strictly needed.

I can fix it myself, no problem.

> Thanks!
> metze



-- 
Luiz Augusto von Dentz

Re: [PATCH] Bluetooth: SCO: check for codecs->num_codecs == 1 before assigning to sco_pi(sk)->codec

[Stefan Metzmacher]

Hi Luiz,

>>> I guess this is odd because there was no need for this code to exist,
>>> BT_CODEC should have used struct bt_codec rather than struct bt_codecs
>>> since we can only set exactly one codec, that said I don't think we
>>> can change this now thus why we need to perform these checks, anyway
>>> back to your changes why don't we have both checks for num_codecs != 1
>>> && optlen < struct_size(codecs, codecs, codecs->num_codecs)) since the
>>> handling is the same?
>>
>> Should I submit a v2 using this:
>> if (codecs->num_codecs != 1 || optlen < struct_size(codecs, codecs, codecs->num_codecs))
>>
>> Or can you to take it from here?
>> As I don't really care and the patch is not strictly needed.
> 
> I can fix it myself, no problem.

Great! Thanks!
metze