linux-kernel - Re: [PATCH] coredump: fix pipe coredump when core limit is 0

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20110821152545.GA30955@redhat.com>
Date:	Sun, 21 Aug 2011 17:25:45 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	bookjovi@...il.com
Cc:	dhowells@...hat.com, nhorman@...driver.com, roland@...hat.com,
	viro@...iv.linux.org.uk, akpm@...ux-foundation.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] coredump: fix pipe coredump when core limit is 0

On 08/21, bookjovi@...il.com wrote:
>
> From: Jovi Zhang <bookjovi@...il.com>
>
> Regressing from 2.6.35

Hmm. Thanks Jovi.

> In pipe coredump case, normally core limits are irrelevant,
> since we're not writing to the file system, but core limit 0
> is a special value, kernel should skip the dump when limit is 0.

Hmm. probably yes... although I'd say I do not really know. iirc,
previously RLIMIT_CORE was simply ignored if ispipe. But then we
changed the rules many time.

Yes. See 725eae32df7754044809973034429a47e6035158. This is where
we changed the "limit == 0 && ispipe" behaviour.

> This error intruduced by commit c71354 in 2.6.35, that commit put
> core limit zero check into non-pipe code branch.
>
>     commit c713541125002b8bc9e681af3b09118e771e2d8a
>     Author: Oleg Nesterov <oleg@...hat.com>
>     Date:   Wed May 26 14:43:05 2010 -0700
>
>     coredump: factor out the not-ispipe file checks

Cough. I don't think so ;)

Yes, that patch moves the check, but please note that before the patch
we did

	if ((!ispipe) && (cprm.limit < binfmt->min_coredump))
		goto fail;

so I do not think this patch can make any difference.

I think this was changed by 898b374af6f71041bd3bceebe257e564f3f1d458.

> For non-pipe case, limit 0 also means drop the coredump, so just put
> the zero limit check at do_coredump function begining.

Neil, what do you think? Should we change the code or the comment?

As for the patch, it is not exactly right in any case,

> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -2119,6 +2119,10 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
>  	if (!__get_dumpable(cprm.mm_flags))
>  		goto fail;
>
> +	/* Core limit as 0 should skip the dump */
> +	if (cprm.limit == 0)
> +		goto fail;

Even if we do not dump, we should kill all tasks/threads which use
this ->mm. We shouldn't miss coredump_wait().

To clarify, I don't really know _why_, and probably it makes sense
to change this behaviour. But this needs a separate patch plus
discussion.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/