linux-kernel - Re: [PATCH 4.4 16/30] [media] pvrusb2: reduce stack usage pvr2_eeprom

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]

Date:   Mon, 3 Jul 2017 09:47:43 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Ben Hutchings <ben.hutchings@...ethink.co.uk>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "# 3.4.x" <stable@...r.kernel.org>,
        Hans Verkuil <hans.verkuil@...co.com>,
        Mauro Carvalho Chehab <mchehab@...pensource.com>
Subject: Re: [PATCH 4.4 16/30] [media] pvrusb2: reduce stack usage pvr2_eeprom_analyze()

On Mon, Jul 3, 2017 at 9:31 AM, Greg Kroah-Hartman
<gregkh@...uxfoundation.org> wrote:
> On Thu, Jun 29, 2017 at 05:15:17PM +0100, Ben Hutchings wrote:
>> On Mon, 2017-06-19 at 23:20 +0800, Greg Kroah-Hartman wrote:
>> > 4.4-stable review patch.  If anyone has any objections, please let me know.
>> >
>> > From: Arnd Bergmann <arnd@...db.de>
>> >
>> > commit 6830733d53a4517588e56227b9c8538633f0c496 upstream.
>> >
>> > The driver uses a relatively large data structure on the stack, which
>> > showed up on my radar as we get a warning with the "latent entropy"
>> > GCC plugin:
>> >
>> > drivers/media/usb/pvrusb2/pvrusb2-eeprom.c:153:1: error: the frame size of 1376 bytes is larger than 1152 bytes [-Werror=frame-larger-than=]
>> >
>> > The warning is usually hidden as we raise the warning limit to 2048
>> > when the plugin is enabled, but I'd like to lower that again in the
>> > future, and making this function smaller helps to do that without
>> > build regressions.
>> >
>> > Further analysis shows that putting an 'i2c_client' structure on
>> > the stack is not really supported, as the embedded 'struct device'
>> > is not initialized here, and we are only saved by the fact that
>> > the function that is called here does not use the pointer at all.
>> [...]
>>
>> That is not true in 4.4-stable.  This commit depends on:
>>
>> commit 6037b3ca28f4258d913dbe77248fd77827702ae3
>> Author: Mauro Carvalho Chehab <mchehab@...pensource.com>
>> Date:   Wed Nov 16 14:21:48 2016 -0200
>>
>>     [media] tveeprom: print log messages using pr_foo()
>
> It does?  I don't understand how the two are connected.  Removing
> i2c_client off of the stack is a good thing.  Ah, I see how the pointer
> is used in tveeprom_hauppauge_analog(), but this shouldn't matter here,
> right?

My reading of the two patches is that we actually need at least one
of them to avoid interpreting uninitialized dev->class/bus:  With just my
6830733d53a patch, we replace the uninitialized data with a NULL
pointer, which is handled gracefully in __dev_printk(), while the
6037b3ca28 patch by itself will avoid using the 'dev' pointer completely,
and give a saner output (no "(NULL device)" string or worse).

I think we probably want both of them backported to 4.4, but I don't see a
dependency between them.

        Arnd