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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHmME9oLLKUxm4zk_=kzRAt5Key_G0+dgozVVw+EwUcYJYDRmg@mail.gmail.com>
Date:	Tue, 26 May 2015 15:49:27 +0200
From:	"Jason A. Donenfeld" <Jason@...c4.com>
To:	Dan Carpenter <dan.carpenter@...cle.com>
Cc:	Shigekatsu Tateno <shigekatsu.tateno@...el.com>,
	linux-kernel@...r.kernel.org,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	devel@...verdev.osuosl.org
Subject: Re: [PATCH v2 1/4] ozwpan: Use proper check to prevent heap overflow

On Tue, May 26, 2015 at 3:32 PM, Dan Carpenter <dan.carpenter@...cle.com> 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/