[PATCH] modules: Fix corruption of /proc/kallsyms

[PATCH] modules: Fix corruption of /proc/kallsyms

[Adrian Hunter]

The commit 91fb02f31505 ("module: Move kallsyms support into a separate
file") changed from using strlcpy() to using strscpy() which created a
buffer overflow. That happened because:
 1) an incorrect value was passed as the buffer length
 2) strscpy() (unlike strlcpy()) may copy beyond the length of the
    input string when copying word-by-word.
The assumption was that because it was already known that the strings
being copied would fit in the space available, it was not necessary
to correctly set the buffer length.  strscpy() breaks that assumption
because although it will not touch bytes beyond the given buffer length
it may write bytes beyond the input string length when writing
word-by-word.

The result of the buffer overflow is to corrupt the symbol type
information that follows. e.g.

 $ sudo cat -v /proc/kallsyms | grep '\^' | head
 ffffffffc0615000 ^@ rfcomm_session_get  [rfcomm]
 ffffffffc061c060 ^@ session_list        [rfcomm]
 ffffffffc06150d0 ^@ rfcomm_send_frame   [rfcomm]
 ffffffffc0615130 ^@ rfcomm_make_uih     [rfcomm]
 ffffffffc07ed58d ^@ bnep_exit   [bnep]
 ffffffffc07ec000 ^@ bnep_rx_control     [bnep]
 ffffffffc07ec1a0 ^@ bnep_session        [bnep]
 ffffffffc07e7000 ^@ input_leds_event    [input_leds]
 ffffffffc07e9000 ^@ input_leds_handler  [input_leds]
 ffffffffc07e7010 ^@ input_leds_disconnect       [input_leds]

Notably, the null bytes (represented above by ^@) can confuse tools.

Fix by correcting the buffer length.

Fixes: 91fb02f31505 ("module: Move kallsyms support into a separate file")
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 kernel/module/kallsyms.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c
index 3e11523bc6f6..18c23545b984 100644
--- a/kernel/module/kallsyms.c
+++ b/kernel/module/kallsyms.c
@@ -137,6 +137,7 @@ void layout_symtab(struct module *mod, struct load_info *info)
 	info->symoffs = ALIGN(mod->data_layout.size, symsect->sh_addralign ?: 1);
 	info->stroffs = mod->data_layout.size = info->symoffs + ndst * sizeof(Elf_Sym);
 	mod->data_layout.size += strtab_size;
+	/* Note add_kallsyms() computes strtab_size as core_typeoffs - stroffs */
 	info->core_typeoffs = mod->data_layout.size;
 	mod->data_layout.size += ndst * sizeof(char);
 	mod->data_layout.size = strict_align(mod->data_layout.size);
@@ -169,6 +170,7 @@ void add_kallsyms(struct module *mod, const struct load_info *info)
 	Elf_Sym *dst;
 	char *s;
 	Elf_Shdr *symsec = &info->sechdrs[info->index.sym];
+	unsigned long strtab_size;
 
 	/* Set up to point into init section. */
 	mod->kallsyms = (void __rcu *)mod->init_layout.base +
@@ -190,19 +192,26 @@ void add_kallsyms(struct module *mod, const struct load_info *info)
 	mod->core_kallsyms.symtab = dst = mod->data_layout.base + info->symoffs;
 	mod->core_kallsyms.strtab = s = mod->data_layout.base + info->stroffs;
 	mod->core_kallsyms.typetab = mod->data_layout.base + info->core_typeoffs;
+	strtab_size = info->core_typeoffs - info->stroffs;
 	src = rcu_dereference_sched(mod->kallsyms)->symtab;
 	for (ndst = i = 0; i < rcu_dereference_sched(mod->kallsyms)->num_symtab; i++) {
 		rcu_dereference_sched(mod->kallsyms)->typetab[i] = elf_type(src + i, info);
 		if (i == 0 || is_livepatch_module(mod) ||
 		    is_core_symbol(src + i, info->sechdrs, info->hdr->e_shnum,
 				   info->index.pcpu)) {
+			ssize_t ret;
+
 			mod->core_kallsyms.typetab[ndst] =
 			    rcu_dereference_sched(mod->kallsyms)->typetab[i];
 			dst[ndst] = src[i];
 			dst[ndst++].st_name = s - mod->core_kallsyms.strtab;
-			s += strscpy(s,
-				     &rcu_dereference_sched(mod->kallsyms)->strtab[src[i].st_name],
-				     KSYM_NAME_LEN) + 1;
+			ret = strscpy(s,
+				      &rcu_dereference_sched(mod->kallsyms)->strtab[src[i].st_name],
+				      strtab_size);
+			if (ret < 0)
+				break;
+			s += ret + 1;
+			strtab_size -= ret + 1;
 		}
 	}
 	preempt_enable();
-- 
2.25.1

Re: [PATCH] modules: Fix corruption of /proc/kallsyms

[Luis Chamberlain]

On Fri, Jul 01, 2022 at 12:44:03PM +0300, Adrian Hunter wrote:
> The commit 91fb02f31505 ("module: Move kallsyms support into a separate
> file") changed from using strlcpy() to using strscpy() which created a
> buffer overflow. That happened because:
>  1) an incorrect value was passed as the buffer length
>  2) strscpy() (unlike strlcpy()) may copy beyond the length of the
>     input string when copying word-by-word.
> The assumption was that because it was already known that the strings
> being copied would fit in the space available, it was not necessary
> to correctly set the buffer length.  strscpy() breaks that assumption
> because although it will not touch bytes beyond the given buffer length
> it may write bytes beyond the input string length when writing
> word-by-word.
> 
> The result of the buffer overflow is to corrupt the symbol type
> information that follows. e.g.
> 
>  $ sudo cat -v /proc/kallsyms | grep '\^' | head
>  ffffffffc0615000 ^@ rfcomm_session_get  [rfcomm]
>  ffffffffc061c060 ^@ session_list        [rfcomm]
>  ffffffffc06150d0 ^@ rfcomm_send_frame   [rfcomm]
>  ffffffffc0615130 ^@ rfcomm_make_uih     [rfcomm]
>  ffffffffc07ed58d ^@ bnep_exit   [bnep]
>  ffffffffc07ec000 ^@ bnep_rx_control     [bnep]
>  ffffffffc07ec1a0 ^@ bnep_session        [bnep]
>  ffffffffc07e7000 ^@ input_leds_event    [input_leds]
>  ffffffffc07e9000 ^@ input_leds_handler  [input_leds]
>  ffffffffc07e7010 ^@ input_leds_disconnect       [input_leds]
> 
> Notably, the null bytes (represented above by ^@) can confuse tools.
> 
> Fix by correcting the buffer length.
> 
> Fixes: 91fb02f31505 ("module: Move kallsyms support into a separate file")
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>

Queued up thanks!

  Luis

Re: [PATCH] modules: Fix corruption of /proc/kallsyms

[Adrian Hunter]

On 2/07/22 00:40, Luis Chamberlain wrote:
> On Fri, Jul 01, 2022 at 12:44:03PM +0300, Adrian Hunter wrote:
>> The commit 91fb02f31505 ("module: Move kallsyms support into a separate
>> file") changed from using strlcpy() to using strscpy() which created a
>> buffer overflow. That happened because:
>>  1) an incorrect value was passed as the buffer length
>>  2) strscpy() (unlike strlcpy()) may copy beyond the length of the
>>     input string when copying word-by-word.
>> The assumption was that because it was already known that the strings
>> being copied would fit in the space available, it was not necessary
>> to correctly set the buffer length.  strscpy() breaks that assumption
>> because although it will not touch bytes beyond the given buffer length
>> it may write bytes beyond the input string length when writing
>> word-by-word.
>>
>> The result of the buffer overflow is to corrupt the symbol type
>> information that follows. e.g.
>>
>>  $ sudo cat -v /proc/kallsyms | grep '\^' | head
>>  ffffffffc0615000 ^@ rfcomm_session_get  [rfcomm]
>>  ffffffffc061c060 ^@ session_list        [rfcomm]
>>  ffffffffc06150d0 ^@ rfcomm_send_frame   [rfcomm]
>>  ffffffffc0615130 ^@ rfcomm_make_uih     [rfcomm]
>>  ffffffffc07ed58d ^@ bnep_exit   [bnep]
>>  ffffffffc07ec000 ^@ bnep_rx_control     [bnep]
>>  ffffffffc07ec1a0 ^@ bnep_session        [bnep]
>>  ffffffffc07e7000 ^@ input_leds_event    [input_leds]
>>  ffffffffc07e9000 ^@ input_leds_handler  [input_leds]
>>  ffffffffc07e7010 ^@ input_leds_disconnect       [input_leds]
>>
>> Notably, the null bytes (represented above by ^@) can confuse tools.
>>
>> Fix by correcting the buffer length.
>>
>> Fixes: 91fb02f31505 ("module: Move kallsyms support into a separate file")
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> 
> Queued up thanks!
> 
>   Luis

Thanks for processing this.

I notice it is -rc6 and I do not see it in Linus' tree. This is a fix
for a regression, shouldn't it be included in 5.19?

Re: [PATCH] modules: Fix corruption of /proc/kallsyms

[Luis Chamberlain]

On Mon, Jul 11, 2022 at 10:48:16AM +0300, Adrian Hunter wrote:
> On 2/07/22 00:40, Luis Chamberlain wrote:
> > On Fri, Jul 01, 2022 at 12:44:03PM +0300, Adrian Hunter wrote:
> >> The commit 91fb02f31505 ("module: Move kallsyms support into a separate
> >> file") changed from using strlcpy() to using strscpy() which created a
> >> buffer overflow. That happened because:
> >>  1) an incorrect value was passed as the buffer length
> >>  2) strscpy() (unlike strlcpy()) may copy beyond the length of the
> >>     input string when copying word-by-word.
> >> The assumption was that because it was already known that the strings
> >> being copied would fit in the space available, it was not necessary
> >> to correctly set the buffer length.  strscpy() breaks that assumption
> >> because although it will not touch bytes beyond the given buffer length
> >> it may write bytes beyond the input string length when writing
> >> word-by-word.
> >>
> >> The result of the buffer overflow is to corrupt the symbol type
> >> information that follows. e.g.
> >>
> >>  $ sudo cat -v /proc/kallsyms | grep '\^' | head
> >>  ffffffffc0615000 ^@ rfcomm_session_get  [rfcomm]
> >>  ffffffffc061c060 ^@ session_list        [rfcomm]
> >>  ffffffffc06150d0 ^@ rfcomm_send_frame   [rfcomm]
> >>  ffffffffc0615130 ^@ rfcomm_make_uih     [rfcomm]
> >>  ffffffffc07ed58d ^@ bnep_exit   [bnep]
> >>  ffffffffc07ec000 ^@ bnep_rx_control     [bnep]
> >>  ffffffffc07ec1a0 ^@ bnep_session        [bnep]
> >>  ffffffffc07e7000 ^@ input_leds_event    [input_leds]
> >>  ffffffffc07e9000 ^@ input_leds_handler  [input_leds]
> >>  ffffffffc07e7010 ^@ input_leds_disconnect       [input_leds]
> >>
> >> Notably, the null bytes (represented above by ^@) can confuse tools.
> >>
> >> Fix by correcting the buffer length.
> >>
> >> Fixes: 91fb02f31505 ("module: Move kallsyms support into a separate file")
> >> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> > 
> > Queued up thanks!
> > 
> >   Luis
> 
> Thanks for processing this.
> 
> I notice it is -rc6 and I do not see it in Linus' tree. This is a fix
> for a regression, shouldn't it be included in 5.19?

Yeah sorry this will get to Linus today.

  Luis

Re: [PATCH] modules: Fix corruption of /proc/kallsyms

[Aaron Tomlin]

On Fri 2022-07-01 12:44 +0300, Adrian Hunter wrote:
> The commit 91fb02f31505 ("module: Move kallsyms support into a separate
> file") changed from using strlcpy() to using strscpy() which created a
> buffer overflow. That happened because:
>  1) an incorrect value was passed as the buffer length
>  2) strscpy() (unlike strlcpy()) may copy beyond the length of the
>     input string when copying word-by-word.
> The assumption was that because it was already known that the strings
> being copied would fit in the space available, it was not necessary
> to correctly set the buffer length.  strscpy() breaks that assumption
> because although it will not touch bytes beyond the given buffer length
> it may write bytes beyond the input string length when writing
> word-by-word.
> 
> The result of the buffer overflow is to corrupt the symbol type
> information that follows. e.g.
> 
>  $ sudo cat -v /proc/kallsyms | grep '\^' | head
>  ffffffffc0615000 ^@ rfcomm_session_get  [rfcomm]
>  ffffffffc061c060 ^@ session_list        [rfcomm]
>  ffffffffc06150d0 ^@ rfcomm_send_frame   [rfcomm]
>  ffffffffc0615130 ^@ rfcomm_make_uih     [rfcomm]
>  ffffffffc07ed58d ^@ bnep_exit   [bnep]
>  ffffffffc07ec000 ^@ bnep_rx_control     [bnep]
>  ffffffffc07ec1a0 ^@ bnep_session        [bnep]
>  ffffffffc07e7000 ^@ input_leds_event    [input_leds]
>  ffffffffc07e9000 ^@ input_leds_handler  [input_leds]
>  ffffffffc07e7010 ^@ input_leds_disconnect       [input_leds]
> 
> Notably, the null bytes (represented above by ^@) can confuse tools.
> 
> Fix by correcting the buffer length.
> 
> Fixes: 91fb02f31505 ("module: Move kallsyms support into a separate file")
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  kernel/module/kallsyms.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)

Hi Adrian,

Thanks for following up!


Kind regards,
-- 
Aaron Tomlin