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

[Stephen Brennan <stephen.s.brennan@oracle.com>]

HAGIO KAZUHITO(萩尾　一仁)	<k-hagio-ab@nec.com> writes:

> 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?

Hi Kazu, thanks for taking a look.

You're right, if first is far below the module range, then there would
be a separate bug, and my patch won't address it. I haven't seen a case
where the symbol name points below the module address range, but I
believe it's possible.

The replacement string comes from the ksplice module. So if the ksplice
module gets allocated to an address below the current module
(lm->mod_base), the case would happen.

A simple way to address this would be to abort the loop which calculates
first/last, if we encounter a string which is outside the module address
range. Something like this:

 for (i = first = last = 0; i < ngplsyms; i++) {
 	modsym = (union kernel_symbol *)
 	    (modsymbuf + (i * kernel_symbol_size));
+	if (modsym_name(gpl_syms, modsym, i) < lm->mod_base ||
+            modsym_name(gpl_syms, modsym, i) >= lm->mod_base + lm->mod_size) {
+		first = last = 0;
+		break;
+	}
 	if (!first
 	    || first > modsym_name(gpl_syms, modsym, i))
 		first = modsym_name(gpl_syms, modsym, i);
 	if (modsym_name(gpl_syms, modsym, i) > last)
 		last = modsym_name(gpl_syms, modsym, i);
 }

With this check, the buffer won't be created unless all the symbols are
in the contiguous region, so we don't need my added check from this
patche.

I will send an updated patch if you like this approach.

Thanks again,
Stephen

> 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