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@xxxxxxxxx> 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@xxxxxxx>
> Cc: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx>
> Cc: Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx>
> Cc: Marcel Holtmann <marcel@xxxxxxxxxxxx>
> Cc: David Wei <dw@xxxxxxxxxxx>
> Cc: linux-bluetooth@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Signed-off-by: Stefan Metzmacher <metze@xxxxxxxxx>
> ---
>  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