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

HAGIO KAZUHITO(萩尾　一仁) <k-hagio-ab(a)nec.com&gt; 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(a)oracle.com&gt; >> --- >> >> 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.