> Anyways, given that the patch is quite large (though straightforward), that
> the subsystem doesn't seem to be very actively maintained and that the user
> base is quite small, it is maybe better to mark this no-dsa in stretch and
> jessie.

... but if we manage to trim down upstream's patch to just a few lines,
it could still be worth it.

I have taken upstream's patch and got rid of all type related changes
which don't have any security related impact. In fact they don't solve
the 'negative len' issue, these changes are just equivalent to moving the
size_t cast a few instructions earlier.

These changes might make sense in a refactoring perspective but this is
just noise in our case.

The resulting patch is tiny:

    diff --git a/bt-host.c b/bt-host.c
    index 2f8f631c25..b73a44d07d 100644
    --- a/bt-host.c
    +++ b/bt-host.c
    @@ -113,6 +113,7 @@ static void vhci_host_send(void *opaque,
         static uint8_t buf[4096];
     
         buf[0] = type;
    +    assert((size_t) len < sizeof(buf));
         memcpy(buf + 1, data, len);
     
         while (write(s->fd, buf, len + 1) < 0)
    diff --git a/hw/bt/hci-csr.c b/hw/bt/hci-csr.c
    index 0341ded50c..26bd516d31 100644
    --- a/hw/bt/hci-csr.c
    +++ b/hw/bt/hci-csr.c
    @@ -320,18 +320,18 @@ static int csrhci_write(struct Chardev *chr,
         struct csrhci_s *s = (struct csrhci_s *)chr;
         int total = 0;
     
    -    if (!s->enable)
    +    if (!s->enable || len <= 0)
             return 0;
     
         for (;;) {
             int cnt = MIN(len, s->in_needed - s->in_len);
    -        if (cnt) {
    -            memcpy(s->inpkt + s->in_len, buf, cnt);
    -            s->in_len += cnt;
    -            buf += cnt;
    -            len -= cnt;
    -            total += cnt;
    -        }
    +        assert(cnt > 0);
    +
    +        memcpy(s->inpkt + s->in_len, buf, cnt);
    +        s->in_len += cnt;
    +        buf += cnt;
    +        len -= cnt;
    +        total += cnt;
     
             if (s->in_len < s->in_needed) {
                 break;

3 lines changed, omitting indentation related diff. Given that this
issue might allow host side DoS/memory corruption I don't think this is
exaggerated.

The only think which is still unclear to me is why the patch is checking
using assert(). If these assert() calls are standard ansi ones, then their
failure would stop the whole qemu process which is not exactly what we
want right?

cheers,
 Hugo

-- 
                Hugo Lefeuvre (hle)    |    www.owl.eu.com
RSA4096_ 360B 03B3 BF27 4F4D 7A3F D5E8 14AA 1EB8 A247 3DFD
ed25519_ 37B2 6D38 0B25 B8A2 6B9F 3A65 A36F 5357 5F2D DC4C

Attachment: signature.asc
Description: PGP signature

Reply via email to