linux-kernel - Re: [PATCH 2/3] coredump: fix error handling for replace

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: <20250414-vollrausch-adressieren-fe4afc00bacb@brauner>
Date: Mon, 14 Apr 2025 15:04:29 +0200
From: Christian Brauner <brauner@...nel.org>
To: Oleg Nesterov <oleg@...hat.com>
Cc: linux-fsdevel@...r.kernel.org, Luca Boccassi <luca.boccassi@...il.com>, 
	Lennart Poettering <lennart@...ttering.net>, Daan De Meyer <daan.j.demeyer@...il.com>, 
	Mike Yuan <me@...dnzj.com>, Zbigniew Jędrzejewski-Szmek <zbyszek@...waw.pl>, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] coredump: fix error handling for replace_fd()

On Mon, Apr 14, 2025 at 02:11:56PM +0200, Oleg Nesterov wrote:
> On 04/14, Christian Brauner wrote:
> >
> > The replace_fd() helper returns the file descriptor number on success
> > and a negative error code on failure. The current error handling in
> > umh_pipe_setup() only works because the file descriptor that is replaced
> > is zero but that's pretty volatile. Explicitly check for a negative
> > error code.
> 
> ...
> 
> > @@ -515,6 +517,9 @@ static int umh_pipe_setup(struct subprocess_info *info, struct cred *new)
> >  
> >  	err = replace_fd(0, files[0], 0);
> >  	fput(files[0]);
> > +	if (err < 0)
> > +		return err;
> > +
> >  	/* and disallow core files too */
> >  	current->signal->rlim[RLIMIT_CORE] = (struct rlimit){1, 1};
> 
> The patch looks trivial and correct, but if we do not want to rely on
> the fact that replace_fd(fd => 0) return 0 on sucess, then this patch
> should also do
> 
> 	-	return err;
> 	+	return 0;
> 
> ?
> 
> otherwise this cleanup looks "incomplete" to me.

Ok, done.