[PATCH v2 1/3] mm/vmstat: fix outdated vmstat_text

[PATCH v2 1/3] mm/vmstat: fix outdated vmstat_text

[Jann Horn]

commit 7a9cdebdcc17 ("mm: get rid of vmacache_flush_all() entirely")
removed the VMACACHE_FULL_FLUSHES statistics, but didn't remove the
corresponding entry in vmstat_text. This causes an out-of-bounds access in
vmstat_show().

Luckily this only affects kernels with CONFIG_DEBUG_VM_VMACACHE=y, which is
probably very rare.

Fixes: 7a9cdebdcc17 ("mm: get rid of vmacache_flush_all() entirely")
Cc: stable@vger.kernel.org
Signed-off-by: Jann Horn <jannh@google.com>
---
 mm/vmstat.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/mm/vmstat.c b/mm/vmstat.c
index 8ba0870ecddd..4cea7b8f519d 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -1283,7 +1283,6 @@ const char * const vmstat_text[] = {
 #ifdef CONFIG_DEBUG_VM_VMACACHE
 	"vmacache_find_calls",
 	"vmacache_find_hits",
-	"vmacache_full_flushes",
 #endif
 #ifdef CONFIG_SWAP
 	"swap_ra",
-- 
2.19.0.605.g01d371f741-goog

[PATCH v2 2/3] mm/vmstat: skip NR_TLB_REMOTE_FLUSH* properly

[Jann Horn]

commit 5dd0b16cdaff ("mm/vmstat: Make NR_TLB_REMOTE_FLUSH_RECEIVED
available even on UP") made the availability of the NR_TLB_REMOTE_FLUSH*
counters inside the kernel unconditional to reduce #ifdef soup, but
(either to avoid showing dummy zero counters to userspace, or because that
code was missed) didn't update the vmstat_array, meaning that all following
counters would be shown with incorrect values.

This only affects kernel builds with
CONFIG_VM_EVENT_COUNTERS=y && CONFIG_DEBUG_TLBFLUSH=y && CONFIG_SMP=n.

Fixes: 5dd0b16cdaff ("mm/vmstat: Make NR_TLB_REMOTE_FLUSH_RECEIVED available even on UP")
Cc: stable@vger.kernel.org
Signed-off-by: Jann Horn <jannh@google.com>
---
 mm/vmstat.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/vmstat.c b/mm/vmstat.c
index 4cea7b8f519d..7878da76abf2 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -1275,6 +1275,9 @@ const char * const vmstat_text[] = {
 #ifdef CONFIG_SMP
 	"nr_tlb_remote_flush",
 	"nr_tlb_remote_flush_received",
+#else
+	"", /* nr_tlb_remote_flush */
+	"", /* nr_tlb_remote_flush_received */
 #endif /* CONFIG_SMP */
 	"nr_tlb_local_flush_all",
 	"nr_tlb_local_flush_one",
-- 
2.19.0.605.g01d371f741-goog

[PATCH v2 3/3] mm/vmstat: assert that vmstat_text is in sync with stat_items_size

[Jann Horn]

As evidenced by the previous two patches, having two gigantic arrays that
must manually be kept in sync, including ifdefs, isn't exactly robust.
To make it easier to catch such issues in the future, add a BUILD_BUG_ON().

Signed-off-by: Jann Horn <jannh@google.com>
---
 mm/vmstat.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/vmstat.c b/mm/vmstat.c
index 7878da76abf2..b678c607e490 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -1663,6 +1663,8 @@ static void *vmstat_start(struct seq_file *m, loff_t *pos)
 	stat_items_size += sizeof(struct vm_event_state);
 #endif
 
+	BUILD_BUG_ON(stat_items_size !=
+		     ARRAY_SIZE(vmstat_text) * sizeof(unsigned long));
 	v = kmalloc(stat_items_size, GFP_KERNEL);
 	m->private = v;
 	if (!v)
-- 
2.19.0.605.g01d371f741-goog

Re: [PATCH v2 1/3] mm/vmstat: fix outdated vmstat_text

[Kees Cook]

On Mon, Oct 1, 2018 at 7:31 AM, Jann Horn <jannh@google.com> wrote:
> commit 7a9cdebdcc17 ("mm: get rid of vmacache_flush_all() entirely")
> removed the VMACACHE_FULL_FLUSHES statistics, but didn't remove the
> corresponding entry in vmstat_text. This causes an out-of-bounds access in
> vmstat_show().
>
> Luckily this only affects kernels with CONFIG_DEBUG_VM_VMACACHE=y, which is
> probably very rare.
>
> Fixes: 7a9cdebdcc17 ("mm: get rid of vmacache_flush_all() entirely")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jann Horn <jannh@google.com>

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees


> ---
>  mm/vmstat.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 8ba0870ecddd..4cea7b8f519d 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -1283,7 +1283,6 @@ const char * const vmstat_text[] = {
>  #ifdef CONFIG_DEBUG_VM_VMACACHE
>         "vmacache_find_calls",
>         "vmacache_find_hits",
> -       "vmacache_full_flushes",
>  #endif
>  #ifdef CONFIG_SWAP
>         "swap_ra",
> --
> 2.19.0.605.g01d371f741-goog
>



-- 
Kees Cook
Pixel Security

Re: [PATCH v2 2/3] mm/vmstat: skip NR_TLB_REMOTE_FLUSH* properly

[Kees Cook]

On Mon, Oct 1, 2018 at 7:31 AM, Jann Horn <jannh@google.com> wrote:
> commit 5dd0b16cdaff ("mm/vmstat: Make NR_TLB_REMOTE_FLUSH_RECEIVED
> available even on UP") made the availability of the NR_TLB_REMOTE_FLUSH*
> counters inside the kernel unconditional to reduce #ifdef soup, but
> (either to avoid showing dummy zero counters to userspace, or because that
> code was missed) didn't update the vmstat_array, meaning that all following
> counters would be shown with incorrect values.
>
> This only affects kernel builds with
> CONFIG_VM_EVENT_COUNTERS=y && CONFIG_DEBUG_TLBFLUSH=y && CONFIG_SMP=n.
>
> Fixes: 5dd0b16cdaff ("mm/vmstat: Make NR_TLB_REMOTE_FLUSH_RECEIVED available even on UP")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jann Horn <jannh@google.com>

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  mm/vmstat.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 4cea7b8f519d..7878da76abf2 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -1275,6 +1275,9 @@ const char * const vmstat_text[] = {
>  #ifdef CONFIG_SMP
>         "nr_tlb_remote_flush",
>         "nr_tlb_remote_flush_received",
> +#else
> +       "", /* nr_tlb_remote_flush */
> +       "", /* nr_tlb_remote_flush_received */
>  #endif /* CONFIG_SMP */
>         "nr_tlb_local_flush_all",
>         "nr_tlb_local_flush_one",
> --
> 2.19.0.605.g01d371f741-goog
>



-- 
Kees Cook
Pixel Security

Re: [PATCH v2 3/3] mm/vmstat: assert that vmstat_text is in sync with stat_items_size

[Kees Cook]

On Mon, Oct 1, 2018 at 7:31 AM, Jann Horn <jannh@google.com> wrote:
> As evidenced by the previous two patches, having two gigantic arrays that
> must manually be kept in sync, including ifdefs, isn't exactly robust.
> To make it easier to catch such issues in the future, add a BUILD_BUG_ON().
>
> Signed-off-by: Jann Horn <jannh@google.com>

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  mm/vmstat.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 7878da76abf2..b678c607e490 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -1663,6 +1663,8 @@ static void *vmstat_start(struct seq_file *m, loff_t *pos)
>         stat_items_size += sizeof(struct vm_event_state);
>  #endif
>
> +       BUILD_BUG_ON(stat_items_size !=
> +                    ARRAY_SIZE(vmstat_text) * sizeof(unsigned long));
>         v = kmalloc(stat_items_size, GFP_KERNEL);
>         m->private = v;
>         if (!v)
> --
> 2.19.0.605.g01d371f741-goog
>



-- 
Kees Cook
Pixel Security

Re: [PATCH v2 1/3] mm/vmstat: fix outdated vmstat_text

[Michal Hocko]

On Mon 01-10-18 16:31:36, Jann Horn wrote:
> commit 7a9cdebdcc17 ("mm: get rid of vmacache_flush_all() entirely")
> removed the VMACACHE_FULL_FLUSHES statistics, but didn't remove the
> corresponding entry in vmstat_text. This causes an out-of-bounds access in
> vmstat_show().
> 
> Luckily this only affects kernels with CONFIG_DEBUG_VM_VMACACHE=y, which is
> probably very rare.
> 
> Fixes: 7a9cdebdcc17 ("mm: get rid of vmacache_flush_all() entirely")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jann Horn <jannh@google.com>

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  mm/vmstat.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 8ba0870ecddd..4cea7b8f519d 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -1283,7 +1283,6 @@ const char * const vmstat_text[] = {
>  #ifdef CONFIG_DEBUG_VM_VMACACHE
>  	"vmacache_find_calls",
>  	"vmacache_find_hits",
> -	"vmacache_full_flushes",
>  #endif
>  #ifdef CONFIG_SWAP
>  	"swap_ra",
> -- 
> 2.19.0.605.g01d371f741-goog

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2 2/3] mm/vmstat: skip NR_TLB_REMOTE_FLUSH* properly

[Michal Hocko]

On Mon 01-10-18 16:31:37, Jann Horn wrote:
> commit 5dd0b16cdaff ("mm/vmstat: Make NR_TLB_REMOTE_FLUSH_RECEIVED
> available even on UP") made the availability of the NR_TLB_REMOTE_FLUSH*
> counters inside the kernel unconditional to reduce #ifdef soup, but
> (either to avoid showing dummy zero counters to userspace, or because that
> code was missed) didn't update the vmstat_array, meaning that all following
> counters would be shown with incorrect values.
> 
> This only affects kernel builds with
> CONFIG_VM_EVENT_COUNTERS=y && CONFIG_DEBUG_TLBFLUSH=y && CONFIG_SMP=n.
> 
> Fixes: 5dd0b16cdaff ("mm/vmstat: Make NR_TLB_REMOTE_FLUSH_RECEIVED available even on UP")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jann Horn <jannh@google.com>

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  mm/vmstat.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 4cea7b8f519d..7878da76abf2 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -1275,6 +1275,9 @@ const char * const vmstat_text[] = {
>  #ifdef CONFIG_SMP
>  	"nr_tlb_remote_flush",
>  	"nr_tlb_remote_flush_received",
> +#else
> +	"", /* nr_tlb_remote_flush */
> +	"", /* nr_tlb_remote_flush_received */
>  #endif /* CONFIG_SMP */
>  	"nr_tlb_local_flush_all",
>  	"nr_tlb_local_flush_one",
> -- 
> 2.19.0.605.g01d371f741-goog

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2 3/3] mm/vmstat: assert that vmstat_text is in sync with stat_items_size

[Michal Hocko]

On Mon 01-10-18 16:31:38, Jann Horn wrote:
> As evidenced by the previous two patches, having two gigantic arrays that
> must manually be kept in sync, including ifdefs, isn't exactly robust.
> To make it easier to catch such issues in the future, add a BUILD_BUG_ON().
> 
> Signed-off-by: Jann Horn <jannh@google.com>

We should have done that looong ago. Thanks!
Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  mm/vmstat.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 7878da76abf2..b678c607e490 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -1663,6 +1663,8 @@ static void *vmstat_start(struct seq_file *m, loff_t *pos)
>  	stat_items_size += sizeof(struct vm_event_state);
>  #endif
>  
> +	BUILD_BUG_ON(stat_items_size !=
> +		     ARRAY_SIZE(vmstat_text) * sizeof(unsigned long));
>  	v = kmalloc(stat_items_size, GFP_KERNEL);
>  	m->private = v;
>  	if (!v)
> -- 
> 2.19.0.605.g01d371f741-goog

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2 3/3] mm/vmstat: assert that vmstat_text is in sync with stat_items_size

[Roman Gushchin]

On Mon, Oct 01, 2018 at 04:31:38PM +0200, Jann Horn wrote:
> As evidenced by the previous two patches, having two gigantic arrays that
> must manually be kept in sync, including ifdefs, isn't exactly robust.
> To make it easier to catch such issues in the future, add a BUILD_BUG_ON().
> 
> Signed-off-by: Jann Horn <jannh@google.com>
> ---
>  mm/vmstat.c | 2 ++
>  1 file changed, 2 insertions(+)

I agree with Michal here, we had to do this long time ago.

For patches 1-3:
Acked-by: Roman Gushchin <guro@fb.com>

BTW, don't we want to split this huge array into smaller parts?
This will make the code more clear and easier to modify.

Thank you!