On Mon, Sep 14, 2020 at 7:57 PM Michal Hocko [off-list ref] wrote:

On Mon 14-09-20 19:46:36, Muchun Song wrote:

quoted

On Mon, Sep 14, 2020 at 6:32 PM Michal Hocko [off-list ref] wrote:

quoted

On Mon 14-09-20 17:43:42, Muchun Song wrote:

quoted

On Mon, Sep 14, 2020 at 5:18 PM Michal Hocko [off-list ref] wrote:

quoted

On Mon 14-09-20 12:02:33, Muchun Song wrote:

quoted

On Sun, Sep 13, 2020 at 8:42 AM Andrew Morton [off-list ref] wrote:

quoted

On Sat, 12 Sep 2020 23:51:00 +0800 Muchun Song [off-list ref] wrote:

quoted

The memory_stat_format() returns a format string, but the return buf
may not including the trailing '\0'. So the users may read the buf
out of bounds.

That sounds serious.  Is a cc:stable appropriate?

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.

Thanks for your explanation. Yeah, seq_printf will add \0 if there is a
room for that. So I agree with you that the "Fixes" tag is wrong. There
is nothing to fix. Sorry for the noise.

I think that if someone uses seq_buf_putc(maybe in the feature) at the
end of memory_stat_format(). It will break the rule and there is no \0.
So this patch can just make the code robust but need to change the
commit log and remove the Fixes tag.

Please see my other reply. Adding \0 is not really sufficient. If we
want to have a robust code to handle the small buffer then we need to
make sure that all counters will make it to the userspace. Short output
is simply a broken result. Implementing this properly is certainly
possible, the question is whether this is worth addressing. It is not
like we are adding a lot of output into this file and it is quite likely
that the code is good as it is.

Got it. Thanks.

--
Michal Hocko
SUSE Labs

-- 
Yours,
Muchun