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@xxxxxxxxx> wrote:
>
> Hi Luiz,
>
> > 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?
>
> 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