On Tue, Jan 29, 2019 at 4:46 AM Jens Axboe <axboe@xxxxxxxxx> wrote: > On 1/28/19 7:21 PM, Jann Horn wrote: > > Please create a local copy of the request before parsing it to keep > > the data from changing under you. Additionally, it might make sense to > > annotate every pointer to shared memory with a comment, or something > > like that, to ensure that anyone looking at the code can immediately > > see for which pointers special caution is required on access. > > I took a look at the viability of NOT having to local copy the data, and > I don't think it's too bad. Local copy has a noticeable impact on the > performance, hence I'd really (REALLY) like to avoid it. > > Here's something on top of the current git branch. I think I even went a > bit too far in some areas, but it should hopefully catch the cases where > we might end up double evaluating the parts of the sqe that we depend > on. For most of the sqe reading we don't really care too much. For > instance, the sqe->user_data. If the app changes this field, then it > just gets whatever passed back in cqe->user_data. That's not a kernel > issue. > > For cases like addr/len etc validation, it should be sound. I'll double > check this in the morning as well, and obviously would need to be folded > in along the way. > > I'd appreciate your opinion on this part, if you see any major issues > with it, or if I missed something. The io_sqe_needs_user() checks still look racy. If that helper sees a IORING_OP_READ_FIXED, but then __io_submit_sqe() sees a IORING_OP_READV - especially if this happens in io_sq_wq_submit_work() -, I think you could potentially end up in places like io_import_iovec() without having done the set_fs(USER_DS) and use_mm(), causing the access to potentially occur with KERNEL_DS and a lazy mm.
