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>] [day] [month] [year] [list]
Date:   Tue, 8 Dec 2020 08:50:56 -0700
From:   Jens Axboe <axboe@...nel.dk>
To:     Hillf Danton <hdanton@...a.com>,
        LKML <linux-kernel@...r.kernel.org>
Cc:     io-uring@...r.kernel.org, syzkaller-bugs@...glegroups.com,
        syzbot+71c4697e27c99fddcf17@...kaller.appspotmail.com,
        Pavel Begunkov <asml.silence@...il.com>
Subject: Re: [PATCH] io_uring: fix file leak on creating io ctx

On 12/8/20 3:28 AM, Hillf Danton wrote:
> On Mon, 7 Dec 2020 09:42:21 -0700 Jens Axboe wrote:
>> On 12/7/20 1:15 AM, Hillf Danton wrote:
>>> @@ -9207,12 +9208,14 @@ err_fd:
>>>  #if defined(CONFIG_UNIX)
>>>  	ctx->ring_sock->file = file;
>>>  #endif
>>> -	if (unlikely(io_uring_add_task_file(ctx, file))) {
>>> -		file = ERR_PTR(-ENOMEM);
>>> -		goto err_fd;
>>> +	ret = io_uring_add_task_file(ctx, file);
>>> +	if (ret) {
>>> +		fput(file);
>>> +		put_unused_fd(fd);
>>> +		goto err;
>>>  	}
>>>  	fd_install(ret, file);
>>> -	return ret;
>>> +	return 0;
>>
>> You're installing the return value from io_uring_add_task_file() in the
>> fd table, and then returning '0' for the fd...
> 
> I canot find phrases to describe the stupid mistake in my patch.
> Thank you so much for pointing it out.

This one is still utterly broken, and (again) cannot have been even
tested in the most basic way. So let's focus on not how things are
phrased, but proper patch etiquette:

- Always (ALWAYS) test your patches. There's no excuse for not doing
  so, and you are blacklisting yourself and ruining your reputation
  by sending garbage that doesn't even pass basic functionality.

- If something isn't tested at all, make it VERY clear that this is
  the case. Generally that's done by putting RFC in there and also
  stating that this is just for discussion, it's not a patch that
  is proposed for inclusion.

- Slow down! I see you sent a patch 10 min after this one, with no
  extra notice in there why that was the case. It's clearly because
  you figured out that this hasty send was bad.

I'd really like to get this in for 5.10, but I'd almost feel better
just redoing the patch myself to ensure it doesn't have other silly
errors in there. Don't put yourself in that position.


> @@ -9207,12 +9208,14 @@ err_fd:
>  #if defined(CONFIG_UNIX)
>  	ctx->ring_sock->file = file;
>  #endif
> -	if (unlikely(io_uring_add_task_file(ctx, file))) {
> -		file = ERR_PTR(-ENOMEM);
> -		goto err_fd;
> +	ret = io_uring_add_task_file(ctx, file);
> +	if (ret) {
> +		fput(file);
> +		put_unused_fd(fd);
> +		goto err;
>  	}
> -	fd_install(ret, file);
> -	return ret;
> +	fd_install(fd, file);
> +	return 0;

You're still returning '0' for the fd. 


-- 
Jens Axboe

Powered by blists - more mailing lists