linux-kernel - Re: [PATCH 2/2] fsnotify: use method copy

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]

Message-ID: <20170804051816.GI2063@ZenIV.linux.org.uk>
Date:   Fri, 4 Aug 2017 06:18:16 +0100
From:   Al Viro <viro@...IV.linux.org.uk>
To:     林守磊 <linxiulei@...il.com>
Cc:     石祤 <leilei.lin@...baba-inc.com>,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        zhiche.yy@...baba-inc.com, torvalds@...ux-foundation.org,
        linux-mm@...ck.org
Subject: Re: [PATCH 2/2] fsnotify: use method copy_dname copying filename

On Fri, Aug 04, 2017 at 11:58:41AM +0800, 林守磊 wrote:
> Hi all
> 
> I sent this patch two months ago, then I found CVE from this link last night
> 
>     http://seclists.org/oss-sec/2017/q3/240
> 
> which not only references this patch, but also provides a upstream fix
> 
>     https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=49d31c2f389acfe83417083e1208422b4091cd9
> 
> I was wondering why @viro hadn't noticed this mail (And @viro fixed
> this). I'm new here and nobody,
> trying to do my best to help the this linux community. I was looking
> forword to your reply, because some
> insufficiency may exists in my work, I'd like to learn from you guys
> 
> Thanks and humble enough to wait your reply

Fair enough.  As for the reasons why allocation of name copy is a bad idea,
consider this: only short (embedded) names get overwritten on rename.
External ones (i.e. anything longer than 32 bytes or so) are unmodified
until freed.  And their lifetime is controlled by a refcount, so we can
trivially get a guaranteed to be stable name in that case - all it takes
is bumping the refcount and the name _will_ stay around until we drop
the reference.  So we are left with the case of short names and that
is trivial to deal with - 32-byte array is small enough, so we can bloody
well have it on caller's stack and copy the (short) name there.
That approach avoids all the headache with allocation, allocation failure
handling, etc.  Stack footprint is not much higher (especially compared
to how much idiotify and friends stomp on the stack) and it's obviously
cheaper - we only copy the name in short case and we never go into
allocator.  And it's just as easy to use as "make a dynamic copy" variant
of API...

Al, still buried in packing boxes at the moment...