diff mbox series

[2/3] coredump: fix error handling for replace_fd()

Message ID 20250414-work-coredump-v1-2-6caebc807ff4@kernel.org (mailing list archive)
State New
Headers show
Series coredump: hand a pidfd to the usermode coredump helper | expand

Commit Message

Christian Brauner April 14, 2025, 9:53 a.m. UTC 
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.

Tested-by: Luca Boccassi <luca.boccassi@gmail.com>
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/coredump.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Oleg Nesterov April 14, 2025, 12:11 p.m. UTC | #1 
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.

Oleg.
Christian Brauner April 14, 2025, 1:04 p.m. UTC | #2 
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.
diff mbox series

Patch

diff --git a/fs/coredump.c b/fs/coredump.c
index c33c177a701b..2ed5e6956a09 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -507,7 +507,9 @@  static int umh_pipe_setup(struct subprocess_info *info, struct cred *new)
 {
 	struct file *files[2];
 	struct coredump_params *cp = (struct coredump_params *)info->data;
-	int err = create_pipe_files(files, 0);
+	int err;
+
+	err = create_pipe_files(files, 0);
 	if (err)
 		return err;
 
@@ -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};