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

[Stefan Metzmacher <metze@samba.org>]

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