[PATCH] symbols: handle module symbols outside strbuf

[PATCH] symbols: handle module symbols outside strbuf

[Stephen Brennan]

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.

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

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

[HAGIO KAZUHITO(萩尾 一仁)]

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?

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

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

[Stephen Brennan]

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

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

[HAGIO KAZUHITO(萩尾 一仁)]

On 2023/12/06 11:11, Stephen Brennan wrote:
> 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.

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