Re: [PATCH v2 1/4] ozwpan: Use proper check to prevent heap overflow

[Jason A. Donenfeld]

On Tue, May 26, 2015 at 3:32 PM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> On Tue, May 26, 2015 at 02:17:46PM +0200, Jason A. Donenfeld wrote:
>> +                     data_len = elt->length -
>>                                       sizeof(struct oz_get_desc_rsp) + 1;
>
> This was in the original code, but I wonder where the + 1 comes from.
> Does anyone know?

I know. It's because oz_get_desc_rsp has a 1 byte data member as it's
last element, that's just meant as a placeholder for a variable amount
of data. elt->length is supposed to be the size of the struct elements
plus the total data section, which runs after the struct. But because
of this placeholder goofiness, when we take sizeof we have to subtract
one.

struct oz_get_desc_rsp {
[... bla bla ...]
        u8      data[1];
} PACKED;

This is sort of horrible, but it is what it is. I'd recommend these
security-CRITICAL patches get merged immediately, and then cleaning up
other problems with this driver can be addressed after, preferably by
the maintainer.


>
> To be honest, I would prefer if we just checked:
>
>         if (elt->length < sizeof(struct oz_get_desc_rsp) + 1)
>                 return;
>         data_len = elt->length - sizeof(struct oz_get_desc_rsp) + 1;
>
> Shouldn't there be an upper bound on length?  Shigekatsu?

elt->length is a u8, so the upper bound is 255.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/