Harry Yoo
Aug. 29, 2025, 11:51 a.m. UTC | #1
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.
Found during Code Review
> 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
Kumar, Kaushlendra
Aug. 29, 2025, 1:12 p.m. UTC | #2
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
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