Re: [PATCH] tools/mm/slabinfo: fix buffer overflows in fread operations

["Kumar, Kaushlendra" <kaushlendra.kumar@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4492 bytes --]

On Fri, Aug 29, 2025 at 03:29:47PM +0530, Kaushlendra Kumar wrote:
> The fread() calls in read_slab_obj() and read_debug_slab_obj() can
> read up to sizeof(buffer) bytes, but then unconditionally write a null
> terminator at buffer[l]. If fread() returns sizeof(buffer), this
> writes beyond the allocated buffer boundaries.
>
> Fix by limiting reads to sizeof(buffer) - 1 bytes in both functions,
> ensuring space is always reserved for null termination. This prevents
> buffer overflows while maintaining proper string handling.
>
> Signed-off-by: Kaushlendra Kumar <kaushlendra.kumar@intel.com>
> ---

> Reviewed-by: Harry Yoo <harry.yoo@oracle.com>
>
> A side question, did you observe this while using the tool?
> Perhaps that means we need to make the buffer bigger.

Thanks for the review!

I discovered this issue while testing some local modifications to the code.
For now, let's keep the current buffer size and address the overflow with this fix.

>  tools/mm/slabinfo.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/mm/slabinfo.c b/tools/mm/slabinfo.c index
> 1433eff99feb..1a7f2874c625 100644
> --- a/tools/mm/slabinfo.c
> +++ b/tools/mm/slabinfo.c
> @@ -228,7 +228,7 @@ static unsigned long read_slab_obj(struct slabinfo *s, const char *name)
>           buffer[0] = 0;
>           l = 0;
>     } else {
> -         l = fread(buffer, 1, sizeof(buffer), f);
> +         l = fread(buffer, 1, sizeof(buffer) - 1, f);
>           buffer[l] = 0;
>           fclose(f);
>     }
> @@ -247,7 +247,7 @@ static unsigned long read_debug_slab_obj(struct slabinfo *s, const char *name)
>           buffer[0] = 0;
>           l = 0;
>     } else {
> -         l = fread(buffer, 1, sizeof(buffer), f);
> +         l = fread(buffer, 1, sizeof(buffer) - 1, f);
>           buffer[l] = 0;
>           fclose(f);
>     }
> --
> 2.34.1


________________________________
From: Harry Yoo <harry.yoo@oracle.com>
Sent: Friday, August 29, 2025 5:21 PM
To: Kumar, Kaushlendra <kaushlendra.kumar@intel.com>
Cc: akpm@linux-foundation.org <akpm@linux-foundation.org>; linux-mm@kvack.org <linux-mm@kvack.org>; vbabka@suse.cz <vbabka@suse.cz>; cl@linux.com <cl@linux.com>; rientjes@google.com <rientjes@google.com>; roman.gushchin@linux.dev <roman.gushchin@linux.dev>
Subject: Re: [PATCH] tools/mm/slabinfo: fix buffer overflows in fread operations

On Fri, Aug 29, 2025 at 03:29:47PM +0530, Kaushlendra Kumar wrote:
> The fread() calls in read_slab_obj() and read_debug_slab_obj() can read
> up to sizeof(buffer) bytes, but then unconditionally write a null
> terminator at buffer[l]. If fread() returns sizeof(buffer), this writes
> beyond the allocated buffer boundaries.
>
> Fix by limiting reads to sizeof(buffer) - 1 bytes in both functions,
> ensuring space is always reserved for null termination. This prevents
> buffer overflows while maintaining proper string handling.
>
> Signed-off-by: Kaushlendra Kumar <kaushlendra.kumar@intel.com>
> ---

Reviewed-by: Harry Yoo <harry.yoo@oracle.com>

A side question, did you observe this while using the tool?
Perhaps that means we need to make the buffer bigger.

>  tools/mm/slabinfo.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/mm/slabinfo.c b/tools/mm/slabinfo.c
> index 1433eff99feb..1a7f2874c625 100644
> --- a/tools/mm/slabinfo.c
> +++ b/tools/mm/slabinfo.c
> @@ -228,7 +228,7 @@ static unsigned long read_slab_obj(struct slabinfo *s, const char *name)
>                buffer[0] = 0;
>                l = 0;
>        } else {
> -             l = fread(buffer, 1, sizeof(buffer), f);
> +             l = fread(buffer, 1, sizeof(buffer) - 1, f);
>                buffer[l] = 0;
>                fclose(f);
>        }
> @@ -247,7 +247,7 @@ static unsigned long read_debug_slab_obj(struct slabinfo *s, const char *name)
>                buffer[0] = 0;
>                l = 0;
>        } else {
> -             l = fread(buffer, 1, sizeof(buffer), f);
> +             l = fread(buffer, 1, sizeof(buffer) - 1, f);
>                buffer[l] = 0;
>                fclose(f);
>        }
> --
> 2.34.1

--
Cheers,
Harry / Hyeonggon

[-- Attachment #2: Type: text/html, Size: 15737 bytes --]