netdev - Re: [EXT] [PATCH] net: atlantic: always deep reset on pm op, fixing null deref regression

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:   Thu, 05 May 2022 17:39:02 +0000
From:   Manuel Ullmann <manuel.ullmann@...teo.de>
To:     Jordan Leppert <jordanleppert@...tonmail.com>
Cc:     Igor Russkikh <irusskikh@...vell.com>,
        Manuel Ullmann <labre@...teo.de>, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org, regressions@...ts.linux.dev,
        davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
        Holger Hoffstätte 
        <holger@...lied-asynchrony.com>, koo5 <kolman.jindrich@...il.com>,
        Dmitry Bezrukov <dbezrukov@...vell.com>
Subject: Re: [EXT] [PATCH] net: atlantic: always deep reset on pm op, fixing
 null deref regression

>>
>>
>> > The impact of this regression is the same for resume that I saw on
>> > thaw: the kernel hangs and nothing except SysRq rebooting can be done.
>> >
>> > The null deref occurs at the same position as on thaw.
>> > BUG: kernel NULL pointer dereference
>> > RIP: aq_ring_rx_fill+0xcf/0x210 [atlantic]
>> >
>> > Fixes regression in cbe6c3a8f8f4 ("net: atlantic: invert deep par in
>> > pm functions, preventing null derefs"), where I disabled deep pm
>> > resets in suspend and resume, trying to make sense of the
>> > atl_resume_common deep parameter in the first place.
>> >
>> > It turns out, that atlantic always has to deep reset on pm operations
>> > and the parameter is useless. Even though I expected that and tested
>> > resume, I screwed up by kexec-rebooting into an unpatched kernel, thus
>> > missing the breakage.
>> >
>> > This fixup obsoletes the deep parameter of atl_resume_common, but I
>> > leave the cleanup for the maintainers to post to mainline.
>> >
>> > PS: I'm very sorry for this regression.
>>
>>
>> Hi Manuel,
>>
>> Unfortunately I've missed to review and comment on previous patch - it was too quickly accepted.
>>
>> I'm still in doubt on your fixes, even after rereading the original problem.
>> Is it possible for you to test this with all the possible combinations?
>> suspend/resume with device up/down,
>> hibernate/restore with device up/down?

I confirm that suspend/resume/hibernation/thaw keeps working in all
cases. Thaw would work without the original patch, if the device is down
before hibernation. I also originally described this behaviour on
bugzilla at

https://bugzilla.kernel.org/show_bug.cgi?id=215798

See also Jordan’s confirmation below.

I think, the main reason, why this could break, is, that the deep
parameter had no real impact until the breaking commit. So it was
practically untested, when the allocation/free functions were split.

Another thing, that I tested, was guarding all null pointer references
with null checks, which failed at first, because GCC optimized them
out. I think I have the atlantic tree for this (bad) fix attempt
floating around. I can try to rebase and create a patch from this and
post it to the Github issue, if you are interested.
https://github.com/Aquantia/AQtion/issues/32

Don’t have time for this before the weekend though.

>> I'll try to do the same on our side, but we don't have much resources for that now unfortunately..
>>
>> > Fixes: cbe6c3a8f8f4315b96e46e1a1c70393c06d95a4c
>>
>>
>> That tag format is incorrect I think..

Thanks for pointing that out. Also, are those stable Cc tags correct?
Because I figured, that the x in the documentation could be also the
branch name and not a placeholder. Should I resend the patch, fixing the
tags? Won’t get to it before tomorrow, though.

>> Igor

> With the proposed patch (deep parameter is always true), I've managed to test:
> 1. Hibernate/restore (with device down/up)
> 2. Suspend/resume (with device down/up)
>
> I put the device down with the command:
> sudo ip link set <connection> down
>
> I hope that's correct, if not please let me know correct command.

This should be the correct.

Manuel