Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 14 Sep 2020 12:20:37 +0100
From:   Chris Down <chris@chrisdown.name>
To:     Michal Hocko <mhocko@suse.com>
Cc:     Muchun Song <songmuchun@bytedance.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Vladimir Davydov <vdavydov.dev@gmail.com>,
        Cgroups <cgroups@vger.kernel.org>,
        Linux Memory Management List <linux-mm@kvack.org>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [External] Re: [PATCH] mm: memcontrol: Fix out-of-bounds on the
 buf returned by memory_stat_format
Message-ID: <20200914112037.GA2417148@chrisdown.name>
References: <20200912155100.25578-1-songmuchun@bytedance.com>
 <20200912174241.eeaa771755915f27babf9322@linux-foundation.org>
 <CAMZfGtXNg31+8QLbUMj7f61Yg1Jgt0rPB7VTDE7qoopGCANGjA@mail.gmail.com>
 <20200914091844.GE16999@dhcp22.suse.cz>
 <CAMZfGtXd3DNrW5BPjDosHsz-FUYACGZEOAfAYLwyHdRSpOsqhQ@mail.gmail.com>
 <20200914103205.GI16999@dhcp22.suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <20200914103205.GI16999@dhcp22.suse.cz>
User-Agent: Mutt/1.14.6 (2020-07-11)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Michal Hocko writes:
>> > > Yeah, I think we should cc:stable.
>> >
>> > Is this a real problem? The buffer should contain 36 lines which makes
>> > it more than 100B per line. I strongly suspect we are not able to use
>> > that storage up.
>>
>> Before memory_stat_format() return, we should call seq_buf_putc(&s, '\0').
>> Otherwise, the return buf string has no trailing null('\0'). But users treat buf
>> as a string(and read the string oob). It is wrong. Thanks.
>
>I am not sure I follow you. vsnprintf which is used by seq_printf will
>add \0 if there is a room for that. And I argue there is a lot of room
>in the buffer so a corner case where the buffer gets full doesn't happen
>with the current code.

I don't feel very strongly either way, but in general I agree with Michal. As 
it is this feels quite perfunctory.

If you can demonstrate reading the string out of bounds in a 
userspace-exploitable way -- that is, you can demonstrate one can coerce 
memory.stat to a format where you would read out of bounds -- then we should 
obviously cc stable and keep the Fixes tag, but you have not come close to 
demonstrating this yet.

Otherwise, if you cannot provide any way to read the string out of bounds, 
because the buffer is simply way too big for this to ever happen, this is just 
a code cleanup, and neither Fixes nor stable are appropriate.

So, the question is, which is it? :-)