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/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ