> -----Original Message----- > From: Dave Anderson <anderson(a)redhat.com&gt; > Sent: Wednesday, June 5, 2019 2:56 PM > To: Nuno Das Neves <Nuno.Das(a)microsoft.com&gt; > Cc: Nuno Das Neves <Nuno.Das(a)microsoft.com&gt; > Subject: Fwd: [Crash-utility] [PATCH] Fix unsigned signed comparison > causing segfault for small VMCOREINFO notes > > > > ----- Forwarded Message ----- > From: "Dave Anderson" <anderson(a)redhat.com&gt; > To: "Discussion list for crash utility usage, maintenance and development" > <crash-utility(a)redhat.com&gt; > Sent: Wednesday, June 5, 2019 4:18:02 PM > Subject: Re: [Crash-utility] [PATCH] Fix unsigned signed comparison causing > segfault for small VMCOREINFO notes > > > > ----- Original Message ----- > > Hi, > > > > This is a fix for a signed/unsigned comparison bug in vmcoreinfo_read_string. > > The bug causes a segmentation fault if size_vmcoreinfo + 1 is smaller than > > the length of the key string passed in. > > I suppose that's true, but can you describe the instance where that actually happened? I'm debugging some issues with vm2core (https://github.com/Azure/azure-linux-utils/tree/master/vm2core), a tool for converting Hyper-V snapshots to elf core files that can be analyzed with Crash. There was a problem where versions of Crash newer than 7.1.5 crashed with elf files generated by vm2core due to this issue. > Can you show the actual note contents as shown by "help -D"? Elf64_Nhdr: n_namesz: 11 ("VMCOREINFO") n_descsz: 10 n_type: 0 (unused) VMCOREINFO So it seems the note doesn't really contain valid data as defined in Documentation/kdump/vmcoreinfo.txt. I've done some additional testing and it appears this note isn't needed by Crash at all - I can simply patch vm2core so it doesn't add the note. So, I suppose this fix isn't required to solve my particular issue.

However, could the VMCOREINFO note legitimately contain a single field e.g. "PAGE_SIZE=4096"? If so, this is just 14 characters; 14 + 1 < strlen("NUMBER(KERNEL_IMAGE_SIZE)") - this string is used as the argument to vmcoreinfo_read_string on line 202 of x86_64.c.

Regards, Nuno > > Thanks, > Dave > > > > > > Signed-off-by: Nuno Das Neves <nudasnev(a)microsoft.com&gt; > > --- > > netdump.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/netdump.c b/netdump.c > > index 40f9cde..d257ecd 100644 > > --- a/netdump.c > > +++ b/netdump.c > > @@ -1838,7 +1838,7 @@ vmcoreinfo_read_string(const char *key) > > return NULL; > > > > /* the '+ 1' is the equal sign */ > > - for (i = 0; i < (size_vmcoreinfo - key_length + 1); i++) { > > + for (i = 0; i < (int)(size_vmcoreinfo - key_length + 1); i++) { > > /* > > * We must also check if we're at the beginning of VMCOREINFO > > * or the separating newline is there, and of course if we > > -- > > 1.8.3.1 > > > > -- > > Crash-utility mailing list > > Crash-utility(a)redhat.com > > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.red... > utility&amp;data=02%7C01%7CNuno.Das%40microsoft.com%7C3a91428c54b34ed7855d08d6ea008afe%7C72f988bf86f141af91ab2d7cd0 > 11db47%7C1%7C0%7C636953685382744097&amp;sdata=BVID2b0QYkDmBip0bzPjX%2Fl4vltJtf78JrTE7pnqSIM%3D&amp;reserved=0 > >

-----Original Message-----

Thanks Masa, the patch looks good. And Daisuke, when you update crash-gcore-command package, please also include the aarch64 patch we discussed last December: --- crash-gcore-command-1.3.1/gcore.c.orig +++ crash-gcore-command-1.3.1/gcore.c @@ -501,6 +501,10 @@ static void gcore_offset_table_init(void GCORE_MEMBER_OFFSET_INIT(vfp_hard_struct_fpscr, "vfp_hard_struct", "fpscr"); GCORE_MEMBER_OFFSET_INIT(thread_struct_fpsimd_state, "thread_struct", "fpsimd_state"); GCORE_MEMBER_OFFSET_INIT(thread_struct_tp_value, "thread_struct", "tp_value"); + if (GCORE_INVALID_MEMBER(thread_struct_fpsimd_state)) { + GCORE_ANON_MEMBER_OFFSET_INIT(thread_struct_fpsimd_state, "thread_struct", "uw.fpsimd_state"); + GCORE_ANON_MEMBER_OFFSET_INIT(thread_struct_tp_value, "thread_struct", "uw.tp_value"); + } } static void gcore_size_table_init(void)

Thanks, Dave ----- Original Message ----- > From: Masayoshi Mizuma <m.mizuma(a)jp.fujitsu.com&gt; > > On 4.19 and newer kernel, gcore command fails as following: > > =========================================================== > gcore: invalid structure size: pid_link > FILE: libgcore/gcore_coredump_table.c LINE: 423 FUNCTION: pid_alive() > > [./crash] error trace: 7f4a7a6762b8 => 7f4a7a676e0f => 53b391 => 53b316 > > 53b316: SIZE_verify.part.31+70 > 53b391: SIZE_verify+49 > > gcore: invalid structure size: pid_link > FILE: libgcore/gcore_coredump_table.c LINE: 423 FUNCTION: pid_alive() > > Failed. > =========================================================== > > That is because struct pid_link is removed and struct pid array is added to > struct signal_struct by commit 2c4704756cab ("pids: Move the pgrp and session > pid pointers from task_struct to signal_struct"). > > Get the pointer of struct pid from task_struct->thread_pid or > signal_struct->pids[] to fix the failure. > > Signed-off-by: Masayoshi Mizuma <m mizuma jp fujitsu com> > --- > gcore.c | 4 +++ > libgcore/gcore_coredump_table.c | 48 +++++++++++++++++++++++++++------ > libgcore/gcore_defs.h | 2 ++ > 3 files changed, 46 insertions(+), 8 deletions(-) > > diff --git a/gcore.c b/gcore.c > index 65dd7a4..4e1304f 100644 > --- a/gcore.c > +++ b/gcore.c > @@ -501,6 +501,10 @@ static void gcore_offset_table_init(void) > GCORE_MEMBER_OFFSET_INIT(vfp_hard_struct_fpscr, "vfp_hard_struct", "fpscr"); > GCORE_MEMBER_OFFSET_INIT(thread_struct_fpsimd_state, "thread_struct", "fpsimd_state"); > GCORE_MEMBER_OFFSET_INIT(thread_struct_tp_value, "thread_struct", "tp_value"); > + if (MEMBER_EXISTS("task_struct", "thread_pid")) > + GCORE_MEMBER_OFFSET_INIT(task_struct_thread_pid, "task_struct", "thread_pid"); > + if (MEMBER_EXISTS("signal_struct", "pids")) > + GCORE_MEMBER_OFFSET_INIT(signal_struct_pids, "signal_struct", "pids"); > } > > static void gcore_size_table_init(void) > diff --git a/libgcore/gcore_coredump_table.c b/libgcore/gcore_coredump_table.c > index a073591..4cf9c38 100644 > --- a/libgcore/gcore_coredump_table.c > +++ b/libgcore/gcore_coredump_table.c > @@ -224,7 +224,7 @@ __task_pid_nr_ns(ulong task, enum pid_type type) > sizeof(ns), "__task_pid_nr_ns: ns", gcore_verbose_error_handle()); > > if (pid_alive(task)) { > - ulong pids_type_pid; > + ulong pids_type_pid, signal; > > if (type != PIDTYPE_PID) > readmem(task + MEMBER_OFFSET("task_struct", > @@ -233,10 +233,34 @@ __task_pid_nr_ns(ulong task, enum pid_type type) > "__task_pid_nr_ns: group_leader", > gcore_verbose_error_handle()); > > - readmem(task + OFFSET(task_struct_pids) + type * SIZE(pid_link) > - + OFFSET(pid_link_pid), KVADDR, &pids_type_pid, > - sizeof(pids_type_pid), > - "__task_pid_nr_ns: pids_type_pid", gcore_verbose_error_handle()); > + if (VALID_MEMBER(task_struct_pids)) > + readmem(task + OFFSET(task_struct_pids) + > + type * SIZE(pid_link) + OFFSET(pid_link_pid), > + KVADDR, &pids_type_pid, > + sizeof(pids_type_pid), > + "__task_pid_nr_ns: pids_type_pid", > + gcore_verbose_error_handle()); > + else > + if (type == PIDTYPE_PID) > + readmem(task + GCORE_OFFSET(task_struct_thread_pid), > + KVADDR, &pids_type_pid, > + sizeof(pids_type_pid), > + "__task_pid_nr_ns: pids_type_pid", > + gcore_verbose_error_handle()); > + else { > + readmem(task + OFFSET(task_struct_signal), > + KVADDR, &signal, > + sizeof(signal), > + "__task_pid_nr_ns: signal", > + gcore_verbose_error_handle()); > + > + readmem(signal + GCORE_OFFSET(signal_struct_pids) + > + type * sizeof(void *), > + KVADDR, &pids_type_pid, > + sizeof(pids_type_pid), > + "__task_pid_nr_ns: pids_type_pid", > + gcore_verbose_error_handle()); > + } > > nr = pid_nr_ns(pids_type_pid, ns); > } > @@ -420,9 +444,17 @@ pid_alive(ulong task) > { > pid_t pid; > > - readmem(task + OFFSET(task_struct_pids) + PIDTYPE_PID * SIZE(pid_link) > - + OFFSET(pid_link_pid), KVADDR, &pid, sizeof(pid), "pid_alive", > - gcore_verbose_error_handle()); > + if (VALID_MEMBER(task_struct_pids)) > + readmem(task + OFFSET(task_struct_pids) + > + PIDTYPE_PID * SIZE(pid_link) + OFFSET(pid_link_pid), > + KVADDR, &pid, sizeof(pid), > + "pid_alive", > + gcore_verbose_error_handle()); > + else > + readmem(task + GCORE_OFFSET(task_struct_thread_pid), > + KVADDR, &pid, sizeof(pid), > + "task_struct.thread_pid", > + gcore_verbose_error_handle()); > > return !!pid; > } > diff --git a/libgcore/gcore_defs.h b/libgcore/gcore_defs.h > index 18c4c2c..3c839f9 100644 > --- a/libgcore/gcore_defs.h > +++ b/libgcore/gcore_defs.h > @@ -1077,6 +1077,7 @@ struct gcore_offset_table > long sched_entity_sum_exec_runtime; > long signal_struct_cutime; > long signal_struct_pgrp; > + long signal_struct_pids; > long signal_struct_session; > long signal_struct_stime; > long signal_struct_sum_sched_runtime; > @@ -1090,6 +1091,7 @@ struct gcore_offset_table > long task_struct_static_prio; > long task_struct_uid; > long task_struct_used_math; > + long task_struct_thread_pid; > long thread_info_status; > long thread_info_fpstate; > long thread_info_vfpstate; > -- > 2.18.1 >