On 24/02/2021 17:15, Colin Ian King wrote:
> Hi,
>
> Static analysis on linux-next with Coverity has detected a potential
> issue with the following commit:
>
> commit e5d1bc0a91f16959aa279aa3ee9fdc246d4bb382
> Author: Pavel Begunkov <asml.silence@xxxxxxxxx>
> Date: Wed Feb 10 00:03:23 2021 +0000
>
> io_uring: defer flushing cached reqs
>
>
> The analysis is as follows:
>
> 1679 static struct io_kiocb *io_alloc_req(struct io_ring_ctx *ctx)
> 1680 {
> 1681 struct io_submit_state *state = &ctx->submit_state;
> 1682
> 1. Condition 0 /* !!(8 > sizeof (state->reqs) / sizeof
> (state->reqs[0]) + (int)sizeof (struct io_alloc_req::[unnamed type]))
> */, taking false branch.
>
> 1683 BUILD_BUG_ON(IO_REQ_ALLOC_BATCH > ARRAY_SIZE(state->reqs));
> 1684
>
> 2. Condition !state->free_reqs, taking true branch.
> 3. cond_const: Checking state->free_reqs implies that
> state->free_reqs is 0 on the false branch.
>
> 1685 if (!state->free_reqs) {
> 1686 gfp_t gfp = GFP_KERNEL | __GFP_NOWARN;
> 1687 int ret;
> 1688
>
> 4. Condition io_flush_cached_reqs(ctx), taking true branch.
>
> 1689 if (io_flush_cached_reqs(ctx))
>
> 5. Jumping to label got_req.
>
> 1690 goto got_req;
> 1691
> 1692 ret = kmem_cache_alloc_bulk(req_cachep, gfp,
> IO_REQ_ALLOC_BATCH,
> 1693 state->reqs);
> 1694
> 1695 /*
> 1696 * Bulk alloc is all-or-nothing. If we fail to get a
> batch,
> 1697 * retry single alloc to be on the safe side.
> 1698 */
> 1699 if (unlikely(ret <= 0)) {
> 1700 state->reqs[0] =
> kmem_cache_alloc(req_cachep, gfp);
> 1701 if (!state->reqs[0])
> 1702 return NULL;
> 1703 ret = 1;
> 1704 }
> 1705 state->free_reqs = ret;
> 1706 }
> 1707got_req:
>
> 6. decr: Decrementing state->free_reqs. The value of
> state->free_reqs is now 4294967295.
>
> 1708 state->free_reqs--;
>
> Out-of-bounds read (OVERRUN)
> 7. overrun-local: Overrunning array state->reqs of 32 8-byte
> elements at element index 4294967295 (byte offset 34359738367) using
> index state->free_reqs (which evaluates to 4294967295).
>
> 1709 return state->reqs[state->free_reqs];
> 1710}
>
> If state->free_reqs is zero and io_flush_cached_reqs(ctx) is true, then
It returns true IFF it incremented state->free_reqs, so should be
a false positive. I'll clean the function up a bit for next, might
help the tool.
> we end up on line 1708 decrementing the unsigned int state->free_reqs so
> this wraps around to 4294967295 causing an out of bounds read on
> state->reqs[].
>
> Colin
>
--
Pavel Begunkov
