[PATCH 1/2] coredump: pass siginfo_t* to do_coredump() and below, not merely signr

[PATCH 1/2] coredump: pass siginfo_t* to do_coredump() and below, not merely signr

[Denys Vlasenko]

This is a preparatory patch for the introduction of NT_SIGINFO elf note.

Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>
---
 fs/binfmt_aout.c         |    2 +-
 fs/binfmt_elf.c          |   14 +++++++-------
 fs/binfmt_elf_fdpic.c    |    6 +++---
 fs/binfmt_flat.c         |    2 +-
 fs/coredump.c            |   10 +++++-----
 include/linux/binfmts.h  |    2 +-
 include/linux/coredump.h |    4 ++--
 kernel/signal.c          |    2 +-
 8 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/fs/binfmt_aout.c b/fs/binfmt_aout.c
index d146e18..3a65adf 100644
--- a/fs/binfmt_aout.c
+++ b/fs/binfmt_aout.c
@@ -89,7 +89,7 @@ static int aout_core_dump(struct coredump_params *cprm)
 	current->flags |= PF_DUMPCORE;
        	strncpy(dump.u_comm, current->comm, sizeof(dump.u_comm));
 	dump.u_ar0 = offsetof(struct user, regs);
-	dump.signal = cprm->signr;
+	dump.signal = cprm->info->si_signo;
 	aout_dump_thread(cprm->regs, &dump);
 
 /* If the size of the dump file exceeds the rlimit, then see what would happen
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 1b4efbc..41a40de 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1479,7 +1479,7 @@ static int fill_thread_core_info(struct elf_thread_core_info *t,
 
 static int fill_note_info(struct elfhdr *elf, int phdrs,
 			  struct elf_note_info *info,
-			  long signr, struct pt_regs *regs)
+			  siginfo_t *siginfo, struct pt_regs *regs)
 {
 	struct task_struct *dump_task = current;
 	const struct user_regset_view *view = task_user_regset_view(dump_task);
@@ -1549,7 +1549,7 @@ static int fill_note_info(struct elfhdr *elf, int phdrs,
 	 * Now fill in each thread's information.
 	 */
 	for (t = info->thread; t != NULL; t = t->next)
-		if (!fill_thread_core_info(t, view, signr, &info->size))
+		if (!fill_thread_core_info(t, view, siginfo->si_signo, &info->size))
 			return 0;
 
 	/*
@@ -1712,14 +1712,14 @@ static int elf_note_info_init(struct elf_note_info *info)
 
 static int fill_note_info(struct elfhdr *elf, int phdrs,
 			  struct elf_note_info *info,
-			  long signr, struct pt_regs *regs)
+			  siginfo_t *siginfo, struct pt_regs *regs)
 {
 	struct list_head *t;
 
 	if (!elf_note_info_init(info))
 		return 0;
 
-	if (signr) {
+	if (siginfo->si_signo) {
 		struct core_thread *ct;
 		struct elf_thread_status *ets;
 
@@ -1737,13 +1737,13 @@ static int fill_note_info(struct elfhdr *elf, int phdrs,
 			int sz;
 
 			ets = list_entry(t, struct elf_thread_status, list);
-			sz = elf_dump_thread_status(signr, ets);
+			sz = elf_dump_thread_status(siginfo->si_signo, ets);
 			info->thread_status_size += sz;
 		}
 	}
 	/* now collect the dump for the current */
 	memset(info->prstatus, 0, sizeof(*info->prstatus));
-	fill_prstatus(info->prstatus, current, signr);
+	fill_prstatus(info->prstatus, current, siginfo->si_signo);
 	elf_core_copy_regs(&info->prstatus->pr_reg, regs);
 
 	/* Set up header */
@@ -1950,7 +1950,7 @@ static int elf_core_dump(struct coredump_params *cprm)
 	 * Collect all the non-memory information about the process for the
 	 * notes.  This also sets up the file header.
 	 */
-	if (!fill_note_info(elf, e_phnum, &info, cprm->signr, cprm->regs))
+	if (!fill_note_info(elf, e_phnum, &info, cprm->info, cprm->regs))
 		goto cleanup;
 
 	has_dumped = 1;
diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
index 3d8fae0..6554f54 100644
--- a/fs/binfmt_elf_fdpic.c
+++ b/fs/binfmt_elf_fdpic.c
@@ -1641,7 +1641,7 @@ static int elf_fdpic_core_dump(struct coredump_params *cprm)
 		goto cleanup;
 #endif
 
-	if (cprm->signr) {
+	if (cprm->info->si_signo) {
 		struct core_thread *ct;
 		struct elf_thread_status *tmp;
 
@@ -1660,13 +1660,13 @@ static int elf_fdpic_core_dump(struct coredump_params *cprm)
 			int sz;
 
 			tmp = list_entry(t, struct elf_thread_status, list);
-			sz = elf_dump_thread_status(cprm->signr, tmp);
+			sz = elf_dump_thread_status(cprm->info->si_signo, tmp);
 			thread_status_size += sz;
 		}
 	}
 
 	/* now collect the dump for the current */
-	fill_prstatus(prstatus, current, cprm->signr);
+	fill_prstatus(prstatus, current, cprm->info->si_signo);
 	elf_core_copy_regs(&prstatus->pr_reg, cprm->regs);
 
 	segs = current->mm->map_count;
diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c
index 178cb70..ebe6feb 100644
--- a/fs/binfmt_flat.c
+++ b/fs/binfmt_flat.c
@@ -107,7 +107,7 @@ static struct linux_binfmt flat_format = {
 static int flat_core_dump(struct coredump_params *cprm)
 {
 	printk("Process %s:%d received signr %d and should have core dumped\n",
-			current->comm, current->pid, (int) cprm->signr);
+			current->comm, current->pid, (int) cprm->info->si_signo);
 	return(1);
 }
 
diff --git a/fs/coredump.c b/fs/coredump.c
index 1935b4d..0027ec0 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -463,7 +463,7 @@ static int umh_pipe_setup(struct subprocess_info *info, struct cred *new)
 	return 0;
 }
 
-void do_coredump(long signr, int exit_code, struct pt_regs *regs)
+void do_coredump(siginfo_t *info, struct pt_regs *regs)
 {
 	struct core_state core_state;
 	struct core_name cn;
@@ -477,7 +477,7 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 	bool need_nonrelative = false;
 	static atomic_t core_dump_count = ATOMIC_INIT(0);
 	struct coredump_params cprm = {
-		.signr = signr,
+		.info = info,
 		.regs = regs,
 		.limit = rlimit(RLIMIT_CORE),
 		/*
@@ -488,7 +488,7 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 		.mm_flags = mm->flags,
 	};
 
-	audit_core_dumps(signr);
+	audit_core_dumps(info->si_signo);
 
 	binfmt = mm->binfmt;
 	if (!binfmt || !binfmt->core_dump)
@@ -512,7 +512,7 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 		need_nonrelative = true;
 	}
 
-	retval = coredump_wait(exit_code, &core_state);
+	retval = coredump_wait(info->si_signo, &core_state);
 	if (retval < 0)
 		goto fail_creds;
 
@@ -524,7 +524,7 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 	 */
 	clear_thread_flag(TIF_SIGPENDING);
 
-	ispipe = format_corename(&cn, signr);
+	ispipe = format_corename(&cn, info->si_signo);
 
  	if (ispipe) {
 		int dump_count;
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index 52fb2eb..f2d1445 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -74,7 +74,7 @@ struct linux_binprm {
 
 /* Function parameter for binfmt->coredump */
 struct coredump_params {
-	long signr;
+	siginfo_t *info;
 	struct pt_regs *regs;
 	struct file *file;
 	unsigned long limit;
diff --git a/include/linux/coredump.h b/include/linux/coredump.h
index 42f9752..be939ce 100644
--- a/include/linux/coredump.h
+++ b/include/linux/coredump.h
@@ -12,9 +12,9 @@
 extern int dump_write(struct file *file, const void *addr, int nr);
 extern int dump_seek(struct file *file, loff_t off);
 #ifdef CONFIG_COREDUMP
-extern void do_coredump(long signr, int exit_code, struct pt_regs *regs);
+extern void do_coredump(siginfo_t *info, struct pt_regs *regs);
 #else
-static inline void do_coredump(long signr, int exit_code, struct pt_regs *regs) {}
+static inline void do_coredump(siginfo_t *info, struct pt_regs *regs) {}
 #endif
 
 #endif /* _LINUX_COREDUMP_H */
diff --git a/kernel/signal.c b/kernel/signal.c
index fb4fd72..546f23d 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2370,7 +2370,7 @@ relock:
 			 * first and our do_group_exit call below will use
 			 * that value and ignore the one we pass it.
 			 */
-			do_coredump(info->si_signo, info->si_signo, regs);
+			do_coredump(info, regs);
 		}
 
 		/*
-- 
1.7.7.6

[PATCH 2/2] coredump: add a new elf note with siginfo fields of the signal

[Denys Vlasenko]

Existing PRSTATUS note contains only si_signo, si_code, si_errno fields
from the siginfo of the signal which caused core to be dumped.

There are tools which try to analyze crashes for possible security
implications, and they want to use, among other data, si_addr field
from the SIGSEGV.

This patch adds a new elf note, NT_SIGINFO, which contains
the remaining fields of siginfo_t.

Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>
---
 fs/binfmt_elf.c     |   52 +++++++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/elf.h |    5 ++++
 2 files changed, 55 insertions(+), 2 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 41a40de..5125188d 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1371,6 +1371,45 @@ static void fill_auxv_note(struct memelfnote *note, struct mm_struct *mm)
 	fill_note(note, "CORE", NT_AUXV, i * sizeof(elf_addr_t), auxv);
 }
 
+struct coredump_siginfo {
+/*	int	csi_signo;	in prstatus.pr_info.si_signo instead */
+/*	int	csi_errno;	in prstatus.pr_info.si_errno */
+/*	int	csi_code;	in prstatus.pr_info.si_code */
+	int	csi_pid;	/* PID of sending process */
+	int	csi_uid;	/* Real UID of sending process */
+/*	int	csi_status;	SIGCHLD never kills, field isn't meaningful */
+/*	clock_t	csi_utime;	SIGCHLD never kills, field isn't meaningful */
+/*	clock_t	csi_stime;	SIGCHLD never kills, field isn't meaningful */
+	void	*csi_ptr;	/* union with si_int */
+	int	csi_tid;	/* POSIX.1b timers */
+	int	csi_overrun;	/* POSIX.1b timers */
+	long	csi_band;	/* SIGIO/POLL: band event */
+	int	csi_fd;		/* SIGIO/POLL: file descriptor */
+	void	*csi_addr;	/* SEGV/BUS: address which caused fault */
+	int	csi_trapno;	/* SEGV/BUS */
+	int	csi_addr_lsb;	/* SEGV/BUS: least significant bit of address */
+	/* Can be extended in the future, if siginfo_t is extended */
+};
+
+static void fill_siginfo_note(struct memelfnote *note, struct coredump_siginfo *data, siginfo_t *siginfo)
+{
+	data->csi_pid      = siginfo->si_pid;
+	data->csi_uid      = siginfo->si_uid;
+	data->csi_ptr      = siginfo->si_ptr;
+	data->csi_overrun  = siginfo->si_overrun;
+	data->csi_tid      = siginfo->si_tid;
+	data->csi_band     = siginfo->si_band;
+	data->csi_fd       = siginfo->si_fd;
+	data->csi_addr     = siginfo->si_addr;
+#ifdef __ARCH_SI_TRAPNO
+	data->csi_trapno   = siginfo->si_trapno;
+#endif
+	/* Prevent signed short->int expansion: */
+	data->csi_addr_lsb = (unsigned short)siginfo->si_addr_lsb;
+
+	fill_note(note, "CORE", NT_SIGINFO, sizeof(*data), data);
+}
+
 #ifdef CORE_DUMP_USE_REGSET
 #include <linux/regset.h>
 
@@ -1384,7 +1423,9 @@ struct elf_thread_core_info {
 struct elf_note_info {
 	struct elf_thread_core_info *thread;
 	struct memelfnote psinfo;
+	struct memelfnote siginfo;
 	struct memelfnote auxv;
+	struct coredump_siginfo csigdata;
 	size_t size;
 	int thread_notes;
 };
@@ -1558,6 +1599,9 @@ static int fill_note_info(struct elfhdr *elf, int phdrs,
 	fill_psinfo(psinfo, dump_task->group_leader, dump_task->mm);
 	info->size += notesize(&info->psinfo);
 
+	fill_siginfo_note(&info->siginfo, &info->csigdata, siginfo);
+	info->size += notesize(&info->siginfo);
+
 	fill_auxv_note(&info->auxv, current->mm);
 	info->size += notesize(&info->auxv);
 
@@ -1587,6 +1631,8 @@ static int write_note_info(struct elf_note_info *info,
 
 		if (first && !writenote(&info->psinfo, file, foffset))
 			return 0;
+		if (first && !writenote(&info->siginfo, file, foffset))
+			return 0;
 		if (first && !writenote(&info->auxv, file, foffset))
 			return 0;
 
@@ -1680,6 +1726,7 @@ struct elf_note_info {
 #ifdef ELF_CORE_COPY_XFPREGS
 	elf_fpxregset_t *xfpu;
 #endif
+	struct coredump_siginfo csigdata;
 	int thread_status_size;
 	int numnote;
 };
@@ -1689,8 +1736,8 @@ static int elf_note_info_init(struct elf_note_info *info)
 	memset(info, 0, sizeof(*info));
 	INIT_LIST_HEAD(&info->thread_list);
 
-	/* Allocate space for six ELF notes */
-	info->notes = kmalloc(6 * sizeof(struct memelfnote), GFP_KERNEL);
+	/* Allocate space for ELF notes */
+	info->notes = kmalloc(7 * sizeof(struct memelfnote), GFP_KERNEL);
 	if (!info->notes)
 		return 0;
 	info->psinfo = kmalloc(sizeof(*info->psinfo), GFP_KERNEL);
@@ -1762,6 +1809,7 @@ static int fill_note_info(struct elfhdr *elf, int phdrs,
 
 	info->numnote = 2;
 
+	fill_siginfo_note(&info->notes[info->numnote++], &info->csigdata, siginfo);
 	fill_auxv_note(&info->notes[info->numnote++], current->mm);
 
 	/* Try to dump the FPU. */
diff --git a/include/linux/elf.h b/include/linux/elf.h
index f930b1a..1e63390 100644
--- a/include/linux/elf.h
+++ b/include/linux/elf.h
@@ -372,6 +372,11 @@ typedef struct elf64_shdr {
 #define NT_PRPSINFO	3
 #define NT_TASKSTRUCT	4
 #define NT_AUXV		6
+/*
+ * Note to userspace developers: size of NT_SIGINFO note may increase
+ * in the future to accomodate more fields, don't assume it is fixed!
+ */
+#define NT_SIGINFO      0x53494749
 #define NT_PRXFPREG     0x46e62b7f      /* copied from gdb5.1/include/elf/common.h */
 #define NT_PPC_VMX	0x100		/* PowerPC Altivec/VMX registers */
 #define NT_PPC_SPE	0x101		/* PowerPC SPE/EVR registers */
-- 
1.7.7.6

Re: [PATCH 2/2] coredump: add a new elf note with siginfo fields of the signal

[Oleg Nesterov]

On 09/13, Denys Vlasenko wrote:
>
> This patch adds a new elf note, NT_SIGINFO, which contains
> the remaining fields of siginfo_t.

I can't really comment this patch, but...

> +struct coredump_siginfo {
> +/*	int	csi_signo;	in prstatus.pr_info.si_signo instead */
> +/*	int	csi_errno;	in prstatus.pr_info.si_errno */
> +/*	int	csi_code;	in prstatus.pr_info.si_code */
> +	int	csi_pid;	/* PID of sending process */
> +	int	csi_uid;	/* Real UID of sending process */
> +/*	int	csi_status;	SIGCHLD never kills, field isn't meaningful */
> +/*	clock_t	csi_utime;	SIGCHLD never kills, field isn't meaningful */
> +/*	clock_t	csi_stime;	SIGCHLD never kills, field isn't meaningful */
> +	void	*csi_ptr;	/* union with si_int */
> +	int	csi_tid;	/* POSIX.1b timers */
> +	int	csi_overrun;	/* POSIX.1b timers */
> +	long	csi_band;	/* SIGIO/POLL: band event */
> +	int	csi_fd;		/* SIGIO/POLL: file descriptor */
> +	void	*csi_addr;	/* SEGV/BUS: address which caused fault */
> +	int	csi_trapno;	/* SEGV/BUS */
> +	int	csi_addr_lsb;	/* SEGV/BUS: least significant bit of address */
> +	/* Can be extended in the future, if siginfo_t is extended */
> +};
> +
> +static void fill_siginfo_note(struct memelfnote *note, struct coredump_siginfo *data, siginfo_t *siginfo)
> +{
> +	data->csi_pid      = siginfo->si_pid;
> +	data->csi_uid      = siginfo->si_uid;
> +	data->csi_ptr      = siginfo->si_ptr;
> +	data->csi_overrun  = siginfo->si_overrun;
> +	data->csi_tid      = siginfo->si_tid;
> +	data->csi_band     = siginfo->si_band;
> +	data->csi_fd       = siginfo->si_fd;
> +	data->csi_addr     = siginfo->si_addr;
> +#ifdef __ARCH_SI_TRAPNO
> +	data->csi_trapno   = siginfo->si_trapno;
> +#endif
> +	/* Prevent signed short->int expansion: */
> +	data->csi_addr_lsb = (unsigned short)siginfo->si_addr_lsb;
> +
> +	fill_note(note, "CORE", NT_SIGINFO, sizeof(*data), data);
> +}

I can't understand the layout. struct siginfo is union, for example
si_overrun only makes sense if si_code = SI_TIMER.

Not sure this is right. I think fill_siginfo_note() should either do
memcpy() and let userspace to decode this (raw) info, or this layout
should be unified with copy_siginfo_to_user().

Note also that we do not expose the upper bits of si_code to user-space,
probably coredump should do the same, I dunno.

Oleg.

Re: [PATCH 2/2] coredump: add a new elf note with siginfo fields of the signal

[Oleg Nesterov]

forgot to mention...

On 09/13, Oleg Nesterov wrote:
>
> Not sure this is right. I think fill_siginfo_note() should either do
> memcpy() and let userspace to decode this (raw) info, or this layout
> should be unified with copy_siginfo_to_user().

And note that we simply do not know what this siginfo contains if
si_code < 0. Again, please look at copy_siginfo_to_user().

> Note also that we do not expose the upper bits of si_code to user-space,
> probably coredump should do the same, I dunno.

If it was sent by kernel, I meant.

Oleg.

Re: [PATCH 2/2] coredump: add a new elf note with siginfo fields of the signal

[Roland McGrath]

I agree with Oleg.  If there is an NT_SIGINFO note, it should contain
exactly the siginfo_t layout and data that we otherwise expose to userland
already.  That is, it must match what PTRACE_GETSIGINFO reports, which (I
think) also matches exactly what appears on the stack for a signal
delivery.  Note also that compat_binfmt_elf must deliver the 32-bit version
of siginfo_t, i.e. as copy_siginfo_to_user32 does.


Thanks,
Roland

Re: [PATCH 2/2] coredump: add a new elf note with siginfo fields of the signal

[Pedro Alves]

On 09/13/2012 06:25 PM, Roland McGrath wrote:
> I agree with Oleg.  If there is an NT_SIGINFO note, it should contain
> exactly the siginfo_t layout and data that we otherwise expose to userland
> already.  That is, it must match what PTRACE_GETSIGINFO reports, which (I
> think) also matches exactly what appears on the stack for a signal
> delivery.  

Agreed.  Please.  One note though:

PTRACE_GETSIGINFO always returns the siginfo_t in the architecture/bitness
of the kernel.  IOW, PTRACE_GETSIGINFO on a 32-bit debuggee, running on a
64-bit kernel, returns the siginfo_t in 64-bit layout.  IOW, copy_siginfo_to_user32
doesn't apply).  Or maybe that's the architecture of the tracer, instead of the kernel.
Haven't really checked.  In any case, GDB has to basically duplicate the kernel's
copy_siginfo_to_user/from_user for each supported arch (that does biarch) because
of this, for "(gdb) print $_siginfo".
I wish we had a PTRACE_GETSIGINFO variant that returned the siginfo_t in
layout of the program, not the kernel's..

> Note also that compat_binfmt_elf must deliver the 32-bit version
> of siginfo_t, i.e. as copy_siginfo_to_user32 does.

-- 
Pedro Alves

Re: [PATCH 2/2] coredump: add a new elf note with siginfo fields of the signal

[Denys Vlasenko]

On Thu, Sep 13, 2012 at 5:36 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 09/13, Denys Vlasenko wrote:
>>
>> This patch adds a new elf note, NT_SIGINFO, which contains
>> the remaining fields of siginfo_t.
>
> I can't really comment this patch, but...
>
>> +struct coredump_siginfo {
...
>> +};
...
>
> I can't understand the layout. struct siginfo is union, for example
> si_overrun only makes sense if si_code = SI_TIMER.
>
> Not sure this is right. I think fill_siginfo_note() should either do
> memcpy() and let userspace to decode this (raw) info, or this layout
> should be unified with copy_siginfo_to_user().

The reason I did not do that is two-fold. First, and most important one,
is layout and alignment reasons. This is *coredump file*,
not in-memory structure. While siginfo_t in memory is always on the same
machine, coredump may be looked at on a completely different
architecture. It would be not too hard if the only differences are 32/64
bitness and whether longs need to be aligned as ints or twice that,
but there are many more subtleties when we deal with insane nested
struct/union beast siginfo_t is.

Example #1:

        union {
...
                struct {
                        __kernel_pid_t _pid;    /* sender's pid */
                        __ARCH_SI_UID_T _uid;   /* sender's uid */
                } _kill;

How wide is __ARCH_SI_UID_T?
Are we sure developer working on machine with architecture A
will get it right for arches B, C, D, and obscure one XYZ
he doesn't even know exists?

Example #2: on which architectures si_trapno exists?


Basically, to resolve these problems userspace coredump
parsing code will be forced to carry *per-architecture*
struct siginfo definitions. Which probably means 20+ structs.
A small nightmare. Imagine how many "oh no, our struct
siginfo for (e.g.) Blackfin was broken for last 3 years
and we didn't notice!" bugs there will be...


With my approach, userspace developers won't be haunted
by these problems: in coredump, we use *unambiguous
format*, where all fields exist on all architectures,
all fields are separate (no nested unions) and all fields
are ints or longs (no shorts, no chars).
Basically, only two different layouts will exist:
32-bit one, and 64-bit one.

Yes, we might be wasting a few bytes, but (1) we also _save_
a few bytes by not saving signo/errno/code redundantly,
and (2) a few bytes saved in *coredump* is well in the noise.

> Note also that we do not expose the upper bits of si_code to user-space,
> probably coredump should do the same, I dunno.

Yes, we shouldn't leak data.

-- 
vda

Re: [PATCH 2/2] coredump: add a new elf note with siginfo fields of the signal

[Roland McGrath]

Other ELF notes have per-machine layouts too.  It's far more important to
reduce the total number of ways we have to represent the same information.
On a given machine, we already have a layout and we don't want another.
Debuggers and so forth already know how to handle the per-machine layouts.
The core file is just another way to get the same chunk of data you can get
from the live process.

Re: [PATCH 1/2] coredump: pass siginfo_t* to do_coredump() and below, not merely signr

[Oleg Nesterov]

On 09/13, Denys Vlasenko wrote:
>
> This is a preparatory patch for the introduction of NT_SIGINFO elf note.

I think the changelog should explain what the patch does.

With this patch we pass "siginfo_t *info" instead of "int signr" to
do_coredump() and put it into coredump_params, this "info" will be used
by the next patch. Other changes are simple s/signr/info->si_signo/.

> @@ -524,7 +524,7 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
>  	 */
>  	clear_thread_flag(TIF_SIGPENDING);
>  
> -	ispipe = format_corename(&cn, signr);
> +	ispipe = format_corename(&cn, info->si_signo);

Well, this conflicts with another fix which we discussed privately,
perhaps it would be better to push that simple change first...
But OK, this conflict is trivial.

Reviewed-by: Oleg Nesterov <oleg@redhat.com>

Re: [PATCH 2/2] coredump: add a new elf note with siginfo fields of the signal

[Jonathan M. Foote]

Hello,

I am the author of the CERT 'exploitable' GDB extension (code here: http://www.cert.org/vuls/discovery/triage.html). The extension uses GDB to give developers information about how exploitable an application crash might be. Right now the extension can only supply useful information for live GDB targets. Denys's patches will allow the extension to work on core files as well, which will enable more teams performing crash triage to use the tool.

As a specific example of how this is useful, in the case of an access violation the extension applies heuristics that try to determine if the access violation was due to a read (si_addr == op.source) or a write (si_addr == op.dest). Write access violations _generally_ require less effort to exploit than read access violations, so, depending on what other heuristics can be applied, the extension may consider a write access violation to be more "more exploitable" than a read access violation. This information is helpful to developers who may have large numbers of crashing test cases to deal with and need to decide which ones to address first. 

As it stands, core files do not include si_addr, and so the 'exploitable' GDB extension is unable to produce even the most basic analysis when applied to them. Denys's patch aims to address this issue, and will therefore allow the 'exploitable' extension to produce some useful information when executed against core files. Since core files have become the standard method of communicating crash information in many contexts, these patches will allow for increased application of the 'exploitable' extension and in a small way promote greater software security for Linux applications.

Please consider accepting these patches.

Thanks,
Jonathan