| Message ID | 20200912155100.25578-1-songmuchun@bytedance.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm: memcontrol: Fix out-of-bounds on the buf returned by memory_stat_format | expand |
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?
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.
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.
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@bytedance.com> 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
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. > -- > Michal Hocko > SUSE Labs
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.
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 <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. > -- > 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 <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.
On Mon, Sep 14, 2020 at 7:57 PM Michal Hocko <mhocko@suse.com> wrote: > > 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. Got it. Thanks. > -- > Michal Hocko > SUSE Labs
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)
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@bytedance.com> --- mm/memcontrol.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)