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