On 2023/12/06 11:11, Stephen Brennan wrote:
> HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@xxxxxxx> 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@xxxxxxxxxx>
>>> ---
>>>
>>> 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
--
Crash-utility mailing list -- devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxxxxxxxxxxxx
https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/
Contribution Guidelines: https://github.com/crash-utility/crash/wiki
