Re: [PULL] vhost: cleanups and fixes

On Thu, Nov 01, 2018 at 04:06:19PM -0700, Linus Torvalds wrote:

On Thu, Nov 1, 2018 at 4:00 PM Kees Cook [off-list ref] wrote:

quoted

+       memset(&rsp, 0, sizeof(rsp));
+       rsp.response = VIRTIO_SCSI_S_FUNCTION_REJECTED;
+       resp = vq->iov[out].iov_base;
+       ret = __copy_to_user(resp, &rsp, sizeof(rsp));

Is it actually safe to trust that iov_base has passed an earlier
access_ok() check here? Why not just use copy_to_user() instead?

Good point.

We really should have removed those double-underscore things ages ago.

Well in case of vhost there are a bunch of reasons to keep them.

One is that all access_ok checks take place
way earlier in context of the owner task. Result is saved
and then used for access repeatedly. Skipping reding access_ok twice
did seem to give a small speedup in the past.
In fact I am looking
into switching some of the uses to unsafe_put_user/unsafe_get_user
after doing something like barrier_nospec after the
access_ok checks. Seems to give a measureable speedup.


Another is that the double underscore things actually can be done
in softirq context if you do preempt_disable/preempt_enable.
IIUC Jason's looking into using that to cut down the latency
for when the access is very small.

Also, apart from the address, what about the size? Wouldn't it be
better to use copy_to_iter() rather than implement it badly by hand?

               Linus

Generally size is checked when we retrieve the iov but I will recheck
this case and reply here.