linux-kernel - Re: New warnings with gcc-11

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] [day] [month] [year] [list]

Date:   Mon, 10 May 2021 12:20:01 +0300
From:   Jani Nikula <jani.nikula@...el.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Daniel Vetter <daniel.vetter@...ll.ch>,
        LKML <linux-kernel@...r.kernel.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>
Subject: Re: New warnings with gcc-11

On Sat, 08 May 2021, Linus Torvalds <torvalds@...ux-foundation.org> wrote:
> I have heard nothing about this, and it remains the only warning from
> my allmodconfig build (I have another one for drm compiled with clang,
> but there I at least heard back that a fix exists).
>
> Since I am going to release rc1 tomorrow, and I don't want to release
> it with an ugly compiler warning, I took it upon myself to just fix
> the code:
>
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fec4d42724a1bf3dcba52307e55375fdb967b852
>
> HOWEVER.
>
> That commit fixes the warning, and is at worst harmless. At best it
> fixes an access to random stack memory. But it does smell like
> somebody who actually knows how these arrays work should look at that
> code.
>
> IOW, maybe the code should actually have read 16 bytes from the Event
> Status Indicator? Maybe offset 10 was wrong? Maybe
> drm_dp_channel_eq_ok() should never have taken six bytes to begin
> with?
>
> It's a mystery, and I haven't heard anything otherwise, so there it is.

Fair enough. My bad for not getting this fixed.

The fix is harmless. drm_dp_channel_eq_ok() only ever accesses 3 bytes
instead of 6. I figure the DP_LINK_STATUS_SIZE (=6) is there because in
the normal case you'd read that much, and use a family of functions on
that data, some of which do access the full 6 bytes, some don't.

In our case, we use drm_dp_channel_eq_ok() to check 3 bytes of similarly
encoded data elsewhere in the DPCD address space, and the
DP_LINK_STATUS_SIZE is meaningless there.

The straightforward fix would be to replace
link_status[DP_LINK_STATUS_SIZE] with link_status[3], and that likely
needs changes in dp_link_status() and dp_get_lane_status() as well.


BR,
Jani.


>
>               Linus
>
> On Wed, Apr 28, 2021 at 12:27 AM Jani Nikula <jani.nikula@...el.com> wrote:
>>
>> On Tue, 27 Apr 2021, Linus Torvalds <torvalds@...ux-foundation.org> wrote:
>> > I've updated to Fedora 34 on one of my machines, and it causes a lot
>> > of i915 warnings like
>> >
>> >   drivers/gpu/drm/i915/intel_pm.c: In function ‘ilk_setup_wm_latency’:
>> >   drivers/gpu/drm/i915/intel_pm.c:3059:9: note: referencing argument 3
>> > of type ‘const u16 *’ {aka ‘const short unsigned int *’}
>> >   drivers/gpu/drm/i915/intel_pm.c:2994:13: note: in a call to function
>> > ‘intel_print_wm_latency’
>> >
>> > and the reason is that gcc now seems to look at the argument array
>> > size more, and notices that
>>
>> Arnd Bergmann reported some of these a while back. I think we have some
>> of them fixed in our -next already, but not all. Thanks for the
>> reminder.

-- 
Jani Nikula, Intel Open Source Graphics Center