On Mon 14-09-20 12:02:33, Muchun Song wrote:
> On Sun, Sep 13, 2020 at 8:42 AM Andrew Morton <[email protected]> wrote:
> >
> > On Sat, 12 Sep 2020 23:51:00 +0800 Muchun Song <[email protected]> wrote:
> >
> > > 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.
--
Michal Hocko
SUSE Labs
On Mon, Sep 14, 2020 at 5:18 PM Michal Hocko <[email protected]> wrote:
>
> On Mon 14-09-20 12:02:33, Muchun Song wrote:
> > On Sun, Sep 13, 2020 at 8:42 AM Andrew Morton <[email protected]> wrote:
> > >
> > > On Sat, 12 Sep 2020 23:51:00 +0800 Muchun Song <[email protected]> wrote:
> > >
> > > > 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.
> --
> Michal Hocko
> SUSE Labs
--
Yours,
Muchun
On Mon 14-09-20 17:43:42, Muchun Song wrote:
> On Mon, Sep 14, 2020 at 5:18 PM Michal Hocko <[email protected]> wrote:
> >
> > On Mon 14-09-20 12:02:33, Muchun Song wrote:
> > > On Sun, Sep 13, 2020 at 8:42 AM Andrew Morton <[email protected]> wrote:
> > > >
> > > > On Sat, 12 Sep 2020 23:51:00 +0800 Muchun Song <[email protected]> wrote:
> > > >
> > > > > 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.
--
Michal Hocko
SUSE Labs
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? :-)
On Mon, Sep 14, 2020 at 6:32 PM Michal Hocko <[email protected]> wrote:
>
> On Mon 14-09-20 17:43:42, Muchun Song wrote:
> > On Mon, Sep 14, 2020 at 5:18 PM Michal Hocko <[email protected]> wrote:
> > >
> > > On Mon 14-09-20 12:02:33, Muchun Song wrote:
> > > > On Sun, Sep 13, 2020 at 8:42 AM Andrew Morton <[email protected]> wrote:
> > > > >
> > > > > On Sat, 12 Sep 2020 23:51:00 +0800 Muchun Song <[email protected]> wrote:
> > > > >
> > > > > > 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.
> --
> Michal Hocko
> SUSE Labs
--
Yours,
Muchun
On Mon 14-09-20 19:46:36, Muchun Song wrote:
> On Mon, Sep 14, 2020 at 6:32 PM Michal Hocko <[email protected]> wrote:
> >
> > On Mon 14-09-20 17:43:42, Muchun Song wrote:
> > > On Mon, Sep 14, 2020 at 5:18 PM Michal Hocko <[email protected]> wrote:
> > > >
> > > > On Mon 14-09-20 12:02:33, Muchun Song wrote:
> > > > > On Sun, Sep 13, 2020 at 8:42 AM Andrew Morton <[email protected]> wrote:
> > > > > >
> > > > > > On Sat, 12 Sep 2020 23:51:00 +0800 Muchun Song <[email protected]> wrote:
> > > > > >
> > > > > > > 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.
--
Michal Hocko
SUSE Labs