* [PATCH net] vhost: validate log when IOTLB is enabled
@ 2018-03-29 8:00 ` Jason Wang
0 siblings, 0 replies; 10+ messages in thread
From: Jason Wang @ 2018-03-29 8:00 UTC (permalink / raw)
To: mst, jasowang; +Cc: netdev, linux-kernel, kvm, virtualization
Vq log_base is the userspace address of bitmap which has nothing to do
with IOTLB. So it needs to be validated unconditionally otherwise we
may try use 0 as log_base which may lead to pin pages that will lead
unexpected result (e.g trigger BUG_ON() in set_bit_to_user()).
Fixes: 6b1e6cc7855b0 ("vhost: new device IOTLB API")
Reported-by: syzbot+6304bf97ef436580fede@syzkaller.appspotmail.com
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/vhost.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 5d5a9d9..5320039 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1244,14 +1244,12 @@ static int vq_log_access_ok(struct vhost_virtqueue *vq,
/* Caller should have vq mutex and device mutex */
int vhost_vq_access_ok(struct vhost_virtqueue *vq)
{
- if (vq->iotlb) {
- /* When device IOTLB was used, the access validation
- * will be validated during prefetching.
- */
- return 1;
- }
- return vq_access_ok(vq, vq->num, vq->desc, vq->avail, vq->used) &&
- vq_log_access_ok(vq, vq->log_base);
+ int ret = vq_log_access_ok(vq, vq->log_base);
+
+ if (ret || vq->iotlb)
+ return ret;
+
+ return vq_access_ok(vq, vq->num, vq->desc, vq->avail, vq->used);
}
EXPORT_SYMBOL_GPL(vhost_vq_access_ok);
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH net] vhost: validate log when IOTLB is enabled
2018-03-29 8:00 ` Jason Wang
(?)
@ 2018-03-29 12:48 ` Michael S. Tsirkin
-1 siblings, 0 replies; 10+ messages in thread
From: Michael S. Tsirkin @ 2018-03-29 12:48 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, linux-kernel, kvm, virtualization
On Thu, Mar 29, 2018 at 04:00:04PM +0800, Jason Wang wrote:
> Vq log_base is the userspace address of bitmap which has nothing to do
> with IOTLB. So it needs to be validated unconditionally otherwise we
> may try use 0 as log_base which may lead to pin pages that will lead
> unexpected result (e.g trigger BUG_ON() in set_bit_to_user()).
>
> Fixes: 6b1e6cc7855b0 ("vhost: new device IOTLB API")
> Reported-by: syzbot+6304bf97ef436580fede@syzkaller.appspotmail.com
> Signed-off-by: Jason Wang <jasowang@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
stable material I guess.
> ---
> drivers/vhost/vhost.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 5d5a9d9..5320039 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -1244,14 +1244,12 @@ static int vq_log_access_ok(struct vhost_virtqueue *vq,
> /* Caller should have vq mutex and device mutex */
> int vhost_vq_access_ok(struct vhost_virtqueue *vq)
> {
> - if (vq->iotlb) {
> - /* When device IOTLB was used, the access validation
> - * will be validated during prefetching.
> - */
> - return 1;
> - }
> - return vq_access_ok(vq, vq->num, vq->desc, vq->avail, vq->used) &&
> - vq_log_access_ok(vq, vq->log_base);
> + int ret = vq_log_access_ok(vq, vq->log_base);
> +
> + if (ret || vq->iotlb)
> + return ret;
> +
> + return vq_access_ok(vq, vq->num, vq->desc, vq->avail, vq->used);
> }
> EXPORT_SYMBOL_GPL(vhost_vq_access_ok);
>
> --
> 2.7.4
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH net] vhost: validate log when IOTLB is enabled
2018-03-29 8:00 ` Jason Wang
(?)
(?)
@ 2018-03-29 12:48 ` Michael S. Tsirkin
2018-03-29 20:23 ` David Miller
2018-03-29 20:23 ` David Miller
-1 siblings, 2 replies; 10+ messages in thread
From: Michael S. Tsirkin @ 2018-03-29 12:48 UTC (permalink / raw)
To: Jason Wang; +Cc: kvm, virtualization, netdev, linux-kernel
On Thu, Mar 29, 2018 at 04:00:04PM +0800, Jason Wang wrote:
> Vq log_base is the userspace address of bitmap which has nothing to do
> with IOTLB. So it needs to be validated unconditionally otherwise we
> may try use 0 as log_base which may lead to pin pages that will lead
> unexpected result (e.g trigger BUG_ON() in set_bit_to_user()).
>
> Fixes: 6b1e6cc7855b0 ("vhost: new device IOTLB API")
> Reported-by: syzbot+6304bf97ef436580fede@syzkaller.appspotmail.com
> Signed-off-by: Jason Wang <jasowang@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
stable material I guess.
> ---
> drivers/vhost/vhost.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 5d5a9d9..5320039 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -1244,14 +1244,12 @@ static int vq_log_access_ok(struct vhost_virtqueue *vq,
> /* Caller should have vq mutex and device mutex */
> int vhost_vq_access_ok(struct vhost_virtqueue *vq)
> {
> - if (vq->iotlb) {
> - /* When device IOTLB was used, the access validation
> - * will be validated during prefetching.
> - */
> - return 1;
> - }
> - return vq_access_ok(vq, vq->num, vq->desc, vq->avail, vq->used) &&
> - vq_log_access_ok(vq, vq->log_base);
> + int ret = vq_log_access_ok(vq, vq->log_base);
> +
> + if (ret || vq->iotlb)
> + return ret;
> +
> + return vq_access_ok(vq, vq->num, vq->desc, vq->avail, vq->used);
> }
> EXPORT_SYMBOL_GPL(vhost_vq_access_ok);
>
> --
> 2.7.4
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH net] vhost: validate log when IOTLB is enabled
2018-03-29 12:48 ` Michael S. Tsirkin
@ 2018-03-29 20:23 ` David Miller
2018-03-29 20:23 ` David Miller
1 sibling, 0 replies; 10+ messages in thread
From: David Miller @ 2018-03-29 20:23 UTC (permalink / raw)
To: mst; +Cc: jasowang, kvm, virtualization, netdev, linux-kernel
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Thu, 29 Mar 2018 15:48:22 +0300
> On Thu, Mar 29, 2018 at 04:00:04PM +0800, Jason Wang wrote:
>> Vq log_base is the userspace address of bitmap which has nothing to do
>> with IOTLB. So it needs to be validated unconditionally otherwise we
>> may try use 0 as log_base which may lead to pin pages that will lead
>> unexpected result (e.g trigger BUG_ON() in set_bit_to_user()).
>>
>> Fixes: 6b1e6cc7855b0 ("vhost: new device IOTLB API")
>> Reported-by: syzbot+6304bf97ef436580fede@syzkaller.appspotmail.com
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>
> stable material I guess.
Applied and queued up for -stable.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH net] vhost: validate log when IOTLB is enabled
2018-03-29 12:48 ` Michael S. Tsirkin
2018-03-29 20:23 ` David Miller
@ 2018-03-29 20:23 ` David Miller
1 sibling, 0 replies; 10+ messages in thread
From: David Miller @ 2018-03-29 20:23 UTC (permalink / raw)
To: mst; +Cc: netdev, linux-kernel, kvm, virtualization
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Thu, 29 Mar 2018 15:48:22 +0300
> On Thu, Mar 29, 2018 at 04:00:04PM +0800, Jason Wang wrote:
>> Vq log_base is the userspace address of bitmap which has nothing to do
>> with IOTLB. So it needs to be validated unconditionally otherwise we
>> may try use 0 as log_base which may lead to pin pages that will lead
>> unexpected result (e.g trigger BUG_ON() in set_bit_to_user()).
>>
>> Fixes: 6b1e6cc7855b0 ("vhost: new device IOTLB API")
>> Reported-by: syzbot+6304bf97ef436580fede@syzkaller.appspotmail.com
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>
> stable material I guess.
Applied and queued up for -stable.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] vhost: validate log when IOTLB is enabled
2018-03-29 8:00 ` Jason Wang
` (2 preceding siblings ...)
(?)
@ 2018-03-29 14:44 ` Michael S. Tsirkin
2018-03-30 2:27 ` Jason Wang
-1 siblings, 1 reply; 10+ messages in thread
From: Michael S. Tsirkin @ 2018-03-29 14:44 UTC (permalink / raw)
To: Jason Wang; +Cc: kvm, virtualization, netdev, linux-kernel
On Thu, Mar 29, 2018 at 04:00:04PM +0800, Jason Wang wrote:
> Vq log_base is the userspace address of bitmap which has nothing to do
> with IOTLB. So it needs to be validated unconditionally otherwise we
> may try use 0 as log_base which may lead to pin pages that will lead
> unexpected result (e.g trigger BUG_ON() in set_bit_to_user()).
>
> Fixes: 6b1e6cc7855b0 ("vhost: new device IOTLB API")
> Reported-by: syzbot+6304bf97ef436580fede@syzkaller.appspotmail.com
> Signed-off-by: Jason Wang <jasowang@redhat.com>
One follow-up question:
We still observe that get user pages returns 0 sometimes. While I agree
we should not pass in unvalidated addresses, isn't this worth
documenting?
> ---
> drivers/vhost/vhost.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 5d5a9d9..5320039 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -1244,14 +1244,12 @@ static int vq_log_access_ok(struct vhost_virtqueue *vq,
> /* Caller should have vq mutex and device mutex */
> int vhost_vq_access_ok(struct vhost_virtqueue *vq)
> {
> - if (vq->iotlb) {
> - /* When device IOTLB was used, the access validation
> - * will be validated during prefetching.
> - */
> - return 1;
> - }
> - return vq_access_ok(vq, vq->num, vq->desc, vq->avail, vq->used) &&
> - vq_log_access_ok(vq, vq->log_base);
> + int ret = vq_log_access_ok(vq, vq->log_base);
> +
> + if (ret || vq->iotlb)
> + return ret;
> +
> + return vq_access_ok(vq, vq->num, vq->desc, vq->avail, vq->used);
> }
> EXPORT_SYMBOL_GPL(vhost_vq_access_ok);
>
> --
> 2.7.4
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH net] vhost: validate log when IOTLB is enabled
2018-03-29 14:44 ` Michael S. Tsirkin
@ 2018-03-30 2:27 ` Jason Wang
0 siblings, 0 replies; 10+ messages in thread
From: Jason Wang @ 2018-03-30 2:27 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, virtualization, netdev, linux-kernel
On 2018年03月29日 22:44, Michael S. Tsirkin wrote:
> On Thu, Mar 29, 2018 at 04:00:04PM +0800, Jason Wang wrote:
>> Vq log_base is the userspace address of bitmap which has nothing to do
>> with IOTLB. So it needs to be validated unconditionally otherwise we
>> may try use 0 as log_base which may lead to pin pages that will lead
>> unexpected result (e.g trigger BUG_ON() in set_bit_to_user()).
>>
>> Fixes: 6b1e6cc7855b0 ("vhost: new device IOTLB API")
>> Reported-by:syzbot+6304bf97ef436580fede@syzkaller.appspotmail.com
>> Signed-off-by: Jason Wang<jasowang@redhat.com>
> One follow-up question:
>
> We still observe that get user pages returns 0 sometimes. While I agree
> we should not pass in unvalidated addresses, isn't this worth
> documenting?
>
>
Looking at get_user_pages_fast(), it has:
if (unlikely(!access_ok(write ? VERIFY_WRITE : VERIFY_READ,
(void __user *)start, len)))
return 0;
So this is expected I think.
Thanks
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH net] vhost: validate log when IOTLB is enabled
@ 2018-03-30 2:27 ` Jason Wang
0 siblings, 0 replies; 10+ messages in thread
From: Jason Wang @ 2018-03-30 2:27 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, kvm, virtualization
On 2018年03月29日 22:44, Michael S. Tsirkin wrote:
> On Thu, Mar 29, 2018 at 04:00:04PM +0800, Jason Wang wrote:
>> Vq log_base is the userspace address of bitmap which has nothing to do
>> with IOTLB. So it needs to be validated unconditionally otherwise we
>> may try use 0 as log_base which may lead to pin pages that will lead
>> unexpected result (e.g trigger BUG_ON() in set_bit_to_user()).
>>
>> Fixes: 6b1e6cc7855b0 ("vhost: new device IOTLB API")
>> Reported-by:syzbot+6304bf97ef436580fede@syzkaller.appspotmail.com
>> Signed-off-by: Jason Wang<jasowang@redhat.com>
> One follow-up question:
>
> We still observe that get user pages returns 0 sometimes. While I agree
> we should not pass in unvalidated addresses, isn't this worth
> documenting?
>
>
Looking at get_user_pages_fast(), it has:
if (unlikely(!access_ok(write ? VERIFY_WRITE : VERIFY_READ,
(void __user *)start, len)))
return 0;
So this is expected I think.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] vhost: validate log when IOTLB is enabled
2018-03-29 8:00 ` Jason Wang
` (3 preceding siblings ...)
(?)
@ 2018-03-29 14:44 ` Michael S. Tsirkin
-1 siblings, 0 replies; 10+ messages in thread
From: Michael S. Tsirkin @ 2018-03-29 14:44 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, linux-kernel, kvm, virtualization
On Thu, Mar 29, 2018 at 04:00:04PM +0800, Jason Wang wrote:
> Vq log_base is the userspace address of bitmap which has nothing to do
> with IOTLB. So it needs to be validated unconditionally otherwise we
> may try use 0 as log_base which may lead to pin pages that will lead
> unexpected result (e.g trigger BUG_ON() in set_bit_to_user()).
>
> Fixes: 6b1e6cc7855b0 ("vhost: new device IOTLB API")
> Reported-by: syzbot+6304bf97ef436580fede@syzkaller.appspotmail.com
> Signed-off-by: Jason Wang <jasowang@redhat.com>
One follow-up question:
We still observe that get user pages returns 0 sometimes. While I agree
we should not pass in unvalidated addresses, isn't this worth
documenting?
> ---
> drivers/vhost/vhost.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 5d5a9d9..5320039 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -1244,14 +1244,12 @@ static int vq_log_access_ok(struct vhost_virtqueue *vq,
> /* Caller should have vq mutex and device mutex */
> int vhost_vq_access_ok(struct vhost_virtqueue *vq)
> {
> - if (vq->iotlb) {
> - /* When device IOTLB was used, the access validation
> - * will be validated during prefetching.
> - */
> - return 1;
> - }
> - return vq_access_ok(vq, vq->num, vq->desc, vq->avail, vq->used) &&
> - vq_log_access_ok(vq, vq->log_base);
> + int ret = vq_log_access_ok(vq, vq->log_base);
> +
> + if (ret || vq->iotlb)
> + return ret;
> +
> + return vq_access_ok(vq, vq->num, vq->desc, vq->avail, vq->used);
> }
> EXPORT_SYMBOL_GPL(vhost_vq_access_ok);
>
> --
> 2.7.4
^ permalink raw reply [flat|nested] 10+ messages in thread