NACK: [SRU][F][PATCH v2 0/3] CVE-2023-21400

[Philip Cox]

On 2024-11-25 11:14 p.m., Chengen Du wrote:
> CVE-2023-21400
>
> BugLink: https://bugs.launchpad.net/bugs/2078659
>
> SRU Justification:
>
> [Impact]
> io_commit_cqring() writes the CQ ring tail to make it visible and also triggers any deferred work.
> When a ring is set up with IOPOLL, it doesn't require locking around the CQ ring updates.
> However, if there is deferred work that needs processing, io_queue_deferred() assumes that the completion_lock is held.
> The io_uring subsystem does not properly handle locking for rings with IOPOLL, leading to a double-free vulnerability, which can be exploited as CVE-2023-21400.
>
> [Fix]
> There is a commit that fixed this issue.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fb348857e7b67eefe365052f1423427b66dedbf3
>
> There is no direct upstream commit for this issue, and the patch needs to be reworked to apply to version 5.4.
>
> [Test Plan]
> This is a timing issue that can be triggered by using the liburing library to implement a test program.
> First, set the io_uring_params flag to IORING_SETUP_IOPOLL and open an XFS file with the O_RDWR | O_DIRECT flags, as the XFS filesystem implements the iopoll function hook.
> After setting up io_uring, create two threads in the process: one thread will wait for completion queue events, and the other will continuously send readv and writev requests in sequence.
> The writev requests should include the IOSQE_IO_DRAIN flag to ensure that previous submission queue events are completed first.
>
> The issue arises when writev requests add entries into the defer_list, but the io_iopoll_complete function consumes entries from defer_list without holding the appropriate lock.
>
> [Where problems could occur]
> The problematic call path can be triggered under specific usage scenarios and only affects io_uring functionality.
> If the patch contains any issues, it may lead to a deadlock.
>
> Jens Axboe (1):
>    io_uring: ensure IOPOLL locks around deferred work
>
> Pavel Begunkov (2):
>    io_uring: remove extra check in __io_commit_cqring
>    io_uring: dont kill fasync under completion_lock
>
>   fs/io_uring.c | 41 ++++++++++++++++++++++++++++-------------
>   1 file changed, 28 insertions(+), 13 deletions(-)
>
I have three main concerns about this change.  Fortunately they 
shouldn't require any code changes to address.

First, I understand dropping the io_commit_needs_flush() from the 
io_iopoll_complete() function, and it does look like it would be a 
hassle to implement the same functionality.  It would be nice to have it 
remain the same, but if we cannot do that, I think the change in 
functionality from the original commit should be explained.

This text in the commit message:

(backported from commit 810e401b34c4c4c244d8b93b9947ea5b3d4d49f8 
linux-5.10.y)
[chengen - introduce the __io_commit_cqring_flush function to separate
the logic in io_commit_cqring / confine the completion_lock range.]

should at least point out this change in functionality.

The main commit message states "and have io_iopoll_complete() check if 
we have deferred work" and this is not true anymore.  So, it would be 
good to document this in the commit message.   I don't think you need to 
put the full justification in the commit message, but it should at least 
point out this in what you had to do for the backport.


This brings me to the second issue with the patchset.  This is a [v2], 
but you have not described in the cover letter, or any of the commits 
what is different from [v1].  This would be very helpful.  If you added 
a [v2] section to the cover letter where you described the justification 
in the change in behavior in io_iopoll_complete() that would help 
explain why you dropped the io_commit_needs_flush() check.


The third thing is that this patchset is sneaking in two unrelated 
patches that are not associated with the CVE, but are fixing something 
else in the code.  I don't really like doing that, but I can see why you 
are doing it in this case.  If no one else objects to this, I won't 
either.  The biggest issue I have with this though, is that if you are 
going to do this, you need to explain it.  This should be outlined in 
the cover letter.


I know that all of this stuff is covered in the different responses you 
have sent on the mailing list, but the review should be a self contained 
unit, and contain all the relevant info.   If it wasn't for the first 
issue of the commit message being misleading I would have overlooked the 
second and third, but if you are going to be sending out another review 
with the updated commit message you may as well address the other issues 
as well.