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

Posted by Stefan Metzmacher 2 months, 1 week ago

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: [PATCH] Bluetooth: SCO: check for codecs->num_codecs == 1 before assigning to sco_pi(sk)->codec

Posted by Luiz Augusto von Dentz 2 months, 1 week ago

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

Posted by Stefan Metzmacher 2 months, 1 week ago

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

Posted by Luiz Augusto von Dentz 2 months, 1 week ago

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

Posted by Stefan Metzmacher 2 months, 1 week ago

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