No upstream commit exists for this commit.
The issue was introduced with backporting upstream commit c16bda37594f
("io_uring/poll: allow some retries for poll triggering spuriously").
Memory allocation can possibly fail causing invalid pointer be
dereferenced just before comparing it to NULL value.
Move the pointer check in proper place (upstream has the similar location
of the check). In case the request has REQ_F_POLLED flag up, apoll can't
be NULL so no need to check there.
Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
Signed-off-by: Fedor Pchelkin <[email protected]>
---
io_uring/io_uring.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 445afda927f4..fd799567fc23 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -5792,10 +5792,10 @@ static int io_arm_poll_handler(struct io_kiocb *req)
}
} else {
apoll = kmalloc(sizeof(*apoll), GFP_ATOMIC);
+ if (unlikely(!apoll))
+ return IO_APOLL_ABORTED;
apoll->poll.retries = APOLL_MAX_RETRY;
}
- if (unlikely(!apoll))
- return IO_APOLL_ABORTED;
apoll->double_poll = NULL;
req->apoll = apoll;
req->flags |= REQ_F_POLLED;
--
2.34.1
On 3/16/23 12:56 PM, Fedor Pchelkin wrote:
> No upstream commit exists for this commit.
>
> The issue was introduced with backporting upstream commit c16bda37594f
> ("io_uring/poll: allow some retries for poll triggering spuriously").
>
> Memory allocation can possibly fail causing invalid pointer be
> dereferenced just before comparing it to NULL value.
>
> Move the pointer check in proper place (upstream has the similar location
> of the check). In case the request has REQ_F_POLLED flag up, apoll can't
> be NULL so no need to check there.
>
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
Ah thanks, yes that's a mistake. Looks good to me!
--
Jens Axboe
On Thu, Mar 16, 2023 at 09:56:16PM +0300, Fedor Pchelkin wrote:
> No upstream commit exists for this commit.
>
> The issue was introduced with backporting upstream commit c16bda37594f
> ("io_uring/poll: allow some retries for poll triggering spuriously").
>
> Memory allocation can possibly fail causing invalid pointer be
> dereferenced just before comparing it to NULL value.
>
> Move the pointer check in proper place (upstream has the similar location
> of the check). In case the request has REQ_F_POLLED flag up, apoll can't
> be NULL so no need to check there.
>
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
>
> Signed-off-by: Fedor Pchelkin <[email protected]>
> ---
> io_uring/io_uring.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
> index 445afda927f4..fd799567fc23 100644
> --- a/io_uring/io_uring.c
> +++ b/io_uring/io_uring.c
> @@ -5792,10 +5792,10 @@ static int io_arm_poll_handler(struct io_kiocb *req)
> }
> } else {
> apoll = kmalloc(sizeof(*apoll), GFP_ATOMIC);
> + if (unlikely(!apoll))
> + return IO_APOLL_ABORTED;
How can you trigger a GFP_ATOMIC memory failure? If you do, worse
things are about to happen to your system, right?
thanks,
greg k-h
On Thu, Mar 16, 2023 at 08:04:24PM +0100, Greg Kroah-Hartman wrote:
>
> How can you trigger a GFP_ATOMIC memory failure? If you do, worse
> things are about to happen to your system, right?
>
> thanks,
>
> greg k-h
Well, Syzkaller triggers them with fail slab, and that is more for
debugging purposes to detect improper handling of error paths.
I agree that if GFP_ATOMIC memory allocation fails then the system is in
more trouble. Do you mean this is the point not to backport it? Now I'm
actually not quite sure about this... It's not clear to me whether such
things should be backported: on one hand, it is a bug which can actually
occur (yes, in very bad situation), on the other - the whole system is not
good anyway.
On 3/16/23 1:22 PM, Fedor Pchelkin wrote:
> On Thu, Mar 16, 2023 at 08:04:24PM +0100, Greg Kroah-Hartman wrote:
>>
>> How can you trigger a GFP_ATOMIC memory failure? If you do, worse
>> things are about to happen to your system, right?
>>
>> thanks,
>>
>> greg k-h
>
> Well, Syzkaller triggers them with fail slab, and that is more for
> debugging purposes to detect improper handling of error paths.
>
> I agree that if GFP_ATOMIC memory allocation fails then the system is in
> more trouble. Do you mean this is the point not to backport it? Now I'm
> actually not quite sure about this... It's not clear to me whether such
> things should be backported: on one hand, it is a bug which can actually
> occur (yes, in very bad situation), on the other - the whole system is not
> good anyway.
I agree with both of you - the likelihood of this happening outside of
synthetic error injection is very small, and if it does, the system has
probably gone to shit anyway.
On the other hand, this does bring the code in line with what upstream
is doing, and I think it's worth adding for that reason alone.
--
Jens Axboe