| Message ID | 5D2859FE-DB39-48F5-BBB5-6EDD3791B6C3@raithlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | io_uring: fix SQPOLL cpu check | expand |
On Tue, Jun 11, 2019 at 11:56:06PM +0000, Stephen Bates wrote: > The array_index_nospec() check in io_sq_offload_start() is performed > before any checks on p->sq_thread_cpu are done. This means cpu is > clamped and therefore no error occurs when out-of-range values are > passed in from userspace. This is in violation of the specification > for io_ring_setup() and causes the io_ring_setup unit test in liburing > to regress. > > Add a new bounds check on sq_thread_cpu at the start of > io_sq_offload_start() so we can exit the function early when bad > values are passed in. > > Fixes: 975554b03edd ("io_uring: fix SQPOLL cpu validation") > Signed-off-by: Stephen Bates <sbates@raithlin.com> Aargh. My original patch [1] handled that correctly, and this case was explicitly called out in the commit message, which was retained even when the patch was "simplified". That's rather disappointing. :/ Thanks, Mark. [1] https://lore.kernel.org/lkml/20190430123451.44227-1-mark.rutland@arm.com/ > --- > fs/io_uring.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fs/io_uring.c b/fs/io_uring.c > index 30a5687..e458470 100644 > --- a/fs/io_uring.c > +++ b/fs/io_uring.c > @@ -2316,6 +2316,9 @@ static int io_sq_offload_start(struct io_ring_ctx *ctx, > { > int ret; > > + if (p->sq_thread_cpu >= nr_cpu_ids) > + return -EINVAL; > + > init_waitqueue_head(&ctx->sqo_wait); > mmgrab(current->mm); > ctx->sqo_mm = current->mm; > -- > 2.7.4 >
> Aargh. My original patch [1] handled that correctly, and this case was > explicitly called out in the commit message, which was retained even > when the patch was "simplified". That's rather disappointing. :/ It looks like Jens did a fix for this (44a9bd18a0f06bba " io_uring: fix failure to verify SQ_AFF cpu") which is in the 5.2-rc series but which hasn’t been applied to the stable series yet. I am not sure how I missed that but it makes my patch redundant. Jens, will 44a9bd18a0f06bba be applied to stable kernels? Stephen
On 6/12/19 3:47 AM, Stephen Bates wrote: >> Aargh. My original patch [1] handled that correctly, and this case was >> explicitly called out in the commit message, which was retained even >> when the patch was "simplified". That's rather disappointing. :/ > > It looks like Jens did a fix for this (44a9bd18a0f06bba > " io_uring: fix failure to verify SQ_AFF cpu") which is in the 5.2-rc series > but which hasn’t been applied to the stable series yet. I am not sure how > I missed that but it makes my patch redundant. > > Jens, will 44a9bd18a0f06bba be applied to stable kernels? Yes, we can get it flagged for stable. Greg, can you pull in the above commit for 5.1 stable?
On Thu, Jun 13, 2019 at 02:54:45AM -0600, Jens Axboe wrote: > On 6/12/19 3:47 AM, Stephen Bates wrote: > >> Aargh. My original patch [1] handled that correctly, and this case was > >> explicitly called out in the commit message, which was retained even > >> when the patch was "simplified". That's rather disappointing. :/ > > > > It looks like Jens did a fix for this (44a9bd18a0f06bba > > " io_uring: fix failure to verify SQ_AFF cpu") which is in the 5.2-rc series > > but which hasn’t been applied to the stable series yet. I am not sure how > > I missed that but it makes my patch redundant. > > > > Jens, will 44a9bd18a0f06bba be applied to stable kernels? > > Yes, we can get it flagged for stable. Greg, can you pull in the above > commit for 5.1 stable? Now snuck in for the next 5.1.y release, thanks. greg k-h
On 6/13/19 3:14 AM, Greg Kroah-Hartman wrote: > On Thu, Jun 13, 2019 at 02:54:45AM -0600, Jens Axboe wrote: >> On 6/12/19 3:47 AM, Stephen Bates wrote: >>>> Aargh. My original patch [1] handled that correctly, and this case was >>>> explicitly called out in the commit message, which was retained even >>>> when the patch was "simplified". That's rather disappointing. :/ >>> >>> It looks like Jens did a fix for this (44a9bd18a0f06bba >>> " io_uring: fix failure to verify SQ_AFF cpu") which is in the 5.2-rc series >>> but which hasn’t been applied to the stable series yet. I am not sure how >>> I missed that but it makes my patch redundant. >>> >>> Jens, will 44a9bd18a0f06bba be applied to stable kernels? >> >> Yes, we can get it flagged for stable. Greg, can you pull in the above >> commit for 5.1 stable? > > Now snuck in for the next 5.1.y release, thanks. Thanks Greg!
diff --git a/fs/io_uring.c b/fs/io_uring.c index 30a5687..e458470 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -2316,6 +2316,9 @@ static int io_sq_offload_start(struct io_ring_ctx *ctx, { int ret; + if (p->sq_thread_cpu >= nr_cpu_ids) + return -EINVAL; + init_waitqueue_head(&ctx->sqo_wait); mmgrab(current->mm); ctx->sqo_mm = current->mm;
The array_index_nospec() check in io_sq_offload_start() is performed before any checks on p->sq_thread_cpu are done. This means cpu is clamped and therefore no error occurs when out-of-range values are passed in from userspace. This is in violation of the specification for io_ring_setup() and causes the io_ring_setup unit test in liburing to regress. Add a new bounds check on sq_thread_cpu at the start of io_sq_offload_start() so we can exit the function early when bad values are passed in. Fixes: 975554b03edd ("io_uring: fix SQPOLL cpu validation") Signed-off-by: Stephen Bates <sbates@raithlin.com> --- fs/io_uring.c | 3 +++ 1 file changed, 3 insertions(+)