Re: [Crash-utility] [PATCH] symbols: handle module symbols outside strbuf

["HAGIO KAZUHITO(萩尾 一仁)" <k-hagio-ab@nec.com>]

On 2023/11/28 23:57, Stephen Brennan wrote:
> Module symbol names can get overwritten by live patches or ksplice in
> odd corner cases, so that the pointer no longer points within the string
> buffer. Gracefully fallback to reading the string directly from the
> kernel image in these cases, to avoid possible segmentation faults
> reading outside the bounds of strbuf.
> 
> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
> ---
> 
> Hi folks - I encountered a segfault on a vmcore which had a module
> symbol that had gotten its name overwritten by a ksplice (live patch).
> It seems like there's not a guarantee that module symbol names _must_
> live within the same symbol buffer, and there is even logic to prevent
> reading too much data into strbuf in those cases.

Thank you for the report and patch.

To me, it seems like the logic is just to cap the buffer size because of 
adding BUFSIZE.

If "last" is outside of a module range, your patch can fix it.  But if 
"first" is far outside (lower) of the module range, strbuflen becomes 
huge and crash tries to allocate a huge memory?  Is it possible by ksplice?

Thanks,
Kazu


> 
> This patch simply ensures that symbol names which start outside of the
> strbuf which we copied, are read directly from the kernel image, rather
> than indexing past the bounds of strbuf. I encountered this in
> store_module_symbols_v2() and have tested it there, but I replicated the
> code to the other versions. I will try to test it out on the other
> variants as well, but I thought I'd share the patch now.
> 
>   symbols.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/symbols.c b/symbols.c
> index 176c950..e70dd69 100644
> --- a/symbols.c
> +++ b/symbols.c
> @@ -1704,7 +1704,7 @@ store_module_symbols_v1(ulong total, int mods_installed)
>   
>   			BZERO(buf1, BUFSIZE);
>   
> -			if (strbuf)
> +			if (strbuf && (unsigned long)modsym->name - first < strbuflen)
>   				strcpy(buf1,
>   					&strbuf[(ulong)modsym->name - first]);
>   			else
> @@ -2080,7 +2080,7 @@ store_module_symbols_6_4(ulong total, int mods_installed)
>   
>   			BZERO(buf1, BUFSIZE);
>   
> -			if (strbuf)
> +			if (strbuf && modsym_name(syms, modsym, i) - first < strbuflen)
>   				strcpy(buf1, &strbuf[modsym_name(syms, modsym, i) - first]);
>   			else
>   				read_string(modsym_name(syms, modsym, i), buf1, BUFSIZE-1);
> @@ -2148,7 +2148,7 @@ store_module_symbols_6_4(ulong total, int mods_installed)
>   
>   			BZERO(buf1, BUFSIZE);
>   
> -			if (strbuf)
> +			if (strbuf && modsym_name(gpl_syms, modsym, i) - first < strbuflen)
>   				strcpy(buf1, &strbuf[modsym_name(gpl_syms, modsym, i) - first]);
>   			else
>   				read_string(modsym_name(gpl_syms, modsym, i), buf1, BUFSIZE-1);
> @@ -2456,7 +2456,7 @@ store_module_symbols_v2(ulong total, int mods_installed)
>   
>   			BZERO(buf1, BUFSIZE);
>   
> -			if (strbuf)
> +			if (strbuf && modsym_name(syms, modsym, i) - first < strbuflen)
>   				strcpy(buf1,
>   					&strbuf[modsym_name(syms, modsym, i) - first]);
>   			else
> @@ -2529,7 +2529,7 @@ store_module_symbols_v2(ulong total, int mods_installed)
>   
>   			BZERO(buf1, BUFSIZE);
>   
> -			if (strbuf)
> +			if (strbuf && modsym_name(gpl_syms, modsym, i) - first < strbuflen)
>   				strcpy(buf1,
>   					&strbuf[modsym_name(gpl_syms, modsym, i) - first]);
>   			else