[PATCH] mm: memcontrol: Fix out-of-bounds on the buf returned by memory_stat_format

[PATCH] mm: memcontrol: Fix out-of-bounds on the buf returned by memory_stat_format

[Muchun Song]

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.

Fixes: c8713d0b2312 ("mm: memcontrol: dump memory.stat during cgroup OOM")
Signed-off-by: Muchun Song <songmuchun-EC8Uxl6Npydl57MIdRCFDg@public.gmane.org>
---
 mm/memcontrol.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index f2ef9a770eeb..20c8a1080074 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1492,12 +1492,13 @@ static bool mem_cgroup_wait_acct_move(struct mem_cgroup *memcg)
 	return false;
 }
 
-static char *memory_stat_format(struct mem_cgroup *memcg)
+static const char *memory_stat_format(struct mem_cgroup *memcg)
 {
 	struct seq_buf s;
 	int i;
 
-	seq_buf_init(&s, kmalloc(PAGE_SIZE, GFP_KERNEL), PAGE_SIZE);
+	/* Reserve a byte for the trailing null */
+	seq_buf_init(&s, kmalloc(PAGE_SIZE, GFP_KERNEL), PAGE_SIZE - 1);
 	if (!s.buffer)
 		return NULL;
 
@@ -1606,7 +1607,8 @@ static char *memory_stat_format(struct mem_cgroup *memcg)
 #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
 
 	/* The above should easily fit into one page */
-	WARN_ON_ONCE(seq_buf_has_overflowed(&s));
+	if (WARN_ON_ONCE(seq_buf_putc(&s, '\0')))
+		s.buffer[PAGE_SIZE - 1] = '\0';
 
 	return s.buffer;
 }
@@ -1644,7 +1646,7 @@ void mem_cgroup_print_oom_context(struct mem_cgroup *memcg, struct task_struct *
  */
 void mem_cgroup_print_oom_meminfo(struct mem_cgroup *memcg)
 {
-	char *buf;
+	const char *buf;
 
 	pr_info("memory: usage %llukB, limit %llukB, failcnt %lu\n",
 		K((u64)page_counter_read(&memcg->memory)),
@@ -6415,7 +6417,7 @@ static int memory_events_local_show(struct seq_file *m, void *v)
 static int memory_stat_show(struct seq_file *m, void *v)
 {
 	struct mem_cgroup *memcg = mem_cgroup_from_seq(m);
-	char *buf;
+	const char *buf;
 
 	buf = memory_stat_format(memcg);
 	if (!buf)
-- 
2.20.1

Re: [PATCH] mm: memcontrol: Fix out-of-bounds on the buf returned by memory_stat_format

[Andrew Morton]

On Sat, 12 Sep 2020 23:51:00 +0800 Muchun Song <songmuchun-EC8Uxl6Npydl57MIdRCFDg@public.gmane.org> 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?

Re: [External] Re: [PATCH] mm: memcontrol: Fix out-of-bounds on the buf returned by memory_stat_format

[Muchun Song]

On Sun, Sep 13, 2020 at 8:42 AM Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> wrote:
>
> On Sat, 12 Sep 2020 23:51:00 +0800 Muchun Song <songmuchun-EC8Uxl6Npydl57MIdRCFDg@public.gmane.org> 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.

-- 
Yours,
Muchun

Re: [External] Re: [PATCH] mm: memcontrol: Fix out-of-bounds on the buf returned by memory_stat_format

[Michal Hocko]

On Mon 14-09-20 12:02:33, Muchun Song wrote:
> On Sun, Sep 13, 2020 at 8:42 AM Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> wrote:
> >
> > On Sat, 12 Sep 2020 23:51:00 +0800 Muchun Song <songmuchun-EC8Uxl6Npydl57MIdRCFDg@public.gmane.org> 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

Re: [External] Re: [PATCH] mm: memcontrol: Fix out-of-bounds on the buf returned by memory_stat_format

[Muchun Song]

On Mon, Sep 14, 2020 at 5:18 PM Michal Hocko <mhocko-IBi9RG/b67k@public.gmane.org> wrote:
>
> On Mon 14-09-20 12:02:33, Muchun Song wrote:
> > On Sun, Sep 13, 2020 at 8:42 AM Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> wrote:
> > >
> > > On Sat, 12 Sep 2020 23:51:00 +0800 Muchun Song <songmuchun-EC8Uxl6Npydl57MIdRCFDg@public.gmane.org> 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

Re: [External] Re: [PATCH] mm: memcontrol: Fix out-of-bounds on the buf returned by memory_stat_format

[Michal Hocko]

On Mon 14-09-20 17:43:42, Muchun Song wrote:
> On Mon, Sep 14, 2020 at 5:18 PM Michal Hocko <mhocko-IBi9RG/b67k@public.gmane.org> wrote:
> >
> > On Mon 14-09-20 12:02:33, Muchun Song wrote:
> > > On Sun, Sep 13, 2020 at 8:42 AM Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> wrote:
> > > >
> > > > On Sat, 12 Sep 2020 23:51:00 +0800 Muchun Song <songmuchun-EC8Uxl6Npydl57MIdRCFDg@public.gmane.org> 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

Re: [External] Re: [PATCH] mm: memcontrol: Fix out-of-bounds on the buf returned by memory_stat_format

[Chris Down]

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? :-)

Re: [External] Re: [PATCH] mm: memcontrol: Fix out-of-bounds on the buf returned by memory_stat_format

[Muchun Song]

On Mon, Sep 14, 2020 at 6:32 PM Michal Hocko <mhocko-IBi9RG/b67k@public.gmane.org> wrote:
>
> On Mon 14-09-20 17:43:42, Muchun Song wrote:
> > On Mon, Sep 14, 2020 at 5:18 PM Michal Hocko <mhocko-IBi9RG/b67k@public.gmane.org> wrote:
> > >
> > > On Mon 14-09-20 12:02:33, Muchun Song wrote:
> > > > On Sun, Sep 13, 2020 at 8:42 AM Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> wrote:
> > > > >
> > > > > On Sat, 12 Sep 2020 23:51:00 +0800 Muchun Song <songmuchun-EC8Uxl6Npydl57MIdRCFDg@public.gmane.org> 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

Re: [External] Re: [PATCH] mm: memcontrol: Fix out-of-bounds on the buf returned by memory_stat_format

[Michal Hocko]

On Mon 14-09-20 19:46:36, Muchun Song wrote:
> On Mon, Sep 14, 2020 at 6:32 PM Michal Hocko <mhocko@suse.com> wrote:
> >
> > On Mon 14-09-20 17:43:42, Muchun Song wrote:
> > > On Mon, Sep 14, 2020 at 5:18 PM Michal Hocko <mhocko@suse.com> wrote:
> > > >
> > > > On Mon 14-09-20 12:02:33, Muchun Song wrote:
> > > > > On Sun, Sep 13, 2020 at 8:42 AM Andrew Morton <akpm@linux-foundation.org> wrote:
> > > > > >
> > > > > > On Sat, 12 Sep 2020 23:51:00 +0800 Muchun Song <songmuchun@bytedance.com> 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

Re: [External] Re: [PATCH] mm: memcontrol: Fix out-of-bounds on the buf returned by memory_stat_format

[Muchun Song]

On Mon, Sep 14, 2020 at 7:57 PM Michal Hocko <mhocko-IBi9RG/b67k@public.gmane.org> wrote:
>
> On Mon 14-09-20 19:46:36, Muchun Song wrote:
> > On Mon, Sep 14, 2020 at 6:32 PM Michal Hocko <mhocko-IBi9RG/b67k@public.gmane.org> wrote:
> > >
> > > On Mon 14-09-20 17:43:42, Muchun Song wrote:
> > > > On Mon, Sep 14, 2020 at 5:18 PM Michal Hocko <mhocko-IBi9RG/b67k@public.gmane.org> wrote:
> > > > >
> > > > > On Mon 14-09-20 12:02:33, Muchun Song wrote:
> > > > > > On Sun, Sep 13, 2020 at 8:42 AM Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> wrote:
> > > > > > >
> > > > > > > On Sat, 12 Sep 2020 23:51:00 +0800 Muchun Song <songmuchun-EC8Uxl6Npydl57MIdRCFDg@public.gmane.org> 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.

Got it. Thanks.

> --
> Michal Hocko
> SUSE Labs



-- 
Yours,
Muchun

Re: [PATCH] mm: memcontrol: Fix out-of-bounds on the buf returned by memory_stat_format

[Michal Hocko]

On Sat 12-09-20 23:51:00, Muchun Song 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.
> 
> Fixes: c8713d0b2312 ("mm: memcontrol: dump memory.stat during cgroup OOM")
> Signed-off-by: Muchun Song <songmuchun-EC8Uxl6Npydl57MIdRCFDg@public.gmane.org>

I would argue that Fixes tag is not appropriate. As already pointed in
other email. There doesn't seem to be any problem currently.

I agree that having the code more robust is reasonable but I am not sure
this patch is the proper answer for that. We do not want to cut the
output as that might confuse userspace consumers. The proper way to
handle this is to flush the content that fits in and process the rest
after that or have a larger buffer.

> ---
>  mm/memcontrol.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index f2ef9a770eeb..20c8a1080074 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1492,12 +1492,13 @@ static bool mem_cgroup_wait_acct_move(struct mem_cgroup *memcg)
>  	return false;
>  }
>  
> -static char *memory_stat_format(struct mem_cgroup *memcg)
> +static const char *memory_stat_format(struct mem_cgroup *memcg)
>  {
>  	struct seq_buf s;
>  	int i;
>  
> -	seq_buf_init(&s, kmalloc(PAGE_SIZE, GFP_KERNEL), PAGE_SIZE);
> +	/* Reserve a byte for the trailing null */
> +	seq_buf_init(&s, kmalloc(PAGE_SIZE, GFP_KERNEL), PAGE_SIZE - 1);
>  	if (!s.buffer)
>  		return NULL;
>  
> @@ -1606,7 +1607,8 @@ static char *memory_stat_format(struct mem_cgroup *memcg)
>  #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
>  
>  	/* The above should easily fit into one page */
> -	WARN_ON_ONCE(seq_buf_has_overflowed(&s));
> +	if (WARN_ON_ONCE(seq_buf_putc(&s, '\0')))
> +		s.buffer[PAGE_SIZE - 1] = '\0';
>  
>  	return s.buffer;
>  }
> @@ -1644,7 +1646,7 @@ void mem_cgroup_print_oom_context(struct mem_cgroup *memcg, struct task_struct *
>   */
>  void mem_cgroup_print_oom_meminfo(struct mem_cgroup *memcg)
>  {
> -	char *buf;
> +	const char *buf;
>  
>  	pr_info("memory: usage %llukB, limit %llukB, failcnt %lu\n",
>  		K((u64)page_counter_read(&memcg->memory)),
> @@ -6415,7 +6417,7 @@ static int memory_events_local_show(struct seq_file *m, void *v)
>  static int memory_stat_show(struct seq_file *m, void *v)
>  {
>  	struct mem_cgroup *memcg = mem_cgroup_from_seq(m);
> -	char *buf;
> +	const char *buf;
>  
>  	buf = memory_stat_format(memcg);
>  	if (!buf)
> -- 
> 2.20.1

-- 
Michal Hocko
SUSE Labs