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

On 2023/12/06 11:11, Stephen Brennan wrote:
> HAGIO KAZUHITO(萩尾　一仁)	<[email protected]> 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 <[email protected]>
>>> ---
>>>
>>> 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.

Thanks for the explanation, understood.

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

Ah, got it.  I guess that usually the number of modules that their 
symbols are overwritten by ksplice is few in a system?  so I'm ok with 
this approach.

I was thinking about continuing here (not abort) if it's not 
IN_MODULE(name, lm) and using strbuf only for strings in the module. 
but maybe we don't need to do such effort if the number is few.  Also it 
will make store_module_symbols_6_4() a bit confusing, so it might be 
overkill.

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

Please go ahead, I think you can use IN_MODULE().

Thanks,
Kazu