While writing a test for a Rust library [1] to handle vhost-vdpa devices,
I experienced the 'use-after-free' issue fixed in patch 1, then I
discovered the potential issue when eventfd_ctx_fdget() fails fixed in
patch 2.
Do you think it might be useful to write a vdpa test suite, perhaps using
this Rust library [2] ?
Could we put it in the kernel tree in tool/testing?
Thanks,
Stefano
[1] https://github.com/stefano-garzarella/vhost/tree/vdpa
[2] https://github.com/rust-vmm/vhost
Stefano Garzarella (2):
vhost-vdpa: fix use-after-free of v->config_ctx
vhost-vdpa: set v->config_ctx to NULL if eventfd_ctx_fdget() fails
drivers/vhost/vdpa.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
--
2.29.2
When the 'v->config_ctx' eventfd_ctx reference is released we didn't
set it to NULL. So if the same character device (e.g. /dev/vhost-vdpa-0)
is re-opened, the 'v->config_ctx' is invalid and calling again
vhost_vdpa_config_put() causes use-after-free issues like the
following refcount_t underflow:
refcount_t: underflow; use-after-free.
WARNING: CPU: 2 PID: 872 at lib/refcount.c:28 refcount_warn_saturate+0xae/0xf0
RIP: 0010:refcount_warn_saturate+0xae/0xf0
Call Trace:
eventfd_ctx_put+0x5b/0x70
vhost_vdpa_release+0xcd/0x150 [vhost_vdpa]
__fput+0x8e/0x240
____fput+0xe/0x10
task_work_run+0x66/0xa0
exit_to_user_mode_prepare+0x118/0x120
syscall_exit_to_user_mode+0x21/0x50
? __x64_sys_close+0x12/0x40
do_syscall_64+0x45/0x50
entry_SYSCALL_64_after_hwframe+0x44/0xae
Fixes: 776f395004d8 ("vhost_vdpa: Support config interrupt in vdpa")
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Stefano Garzarella <[email protected]>
---
drivers/vhost/vdpa.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index ef688c8c0e0e..00796e4ecfdf 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -308,8 +308,10 @@ static long vhost_vdpa_get_vring_num(struct vhost_vdpa *v, u16 __user *argp)
static void vhost_vdpa_config_put(struct vhost_vdpa *v)
{
- if (v->config_ctx)
+ if (v->config_ctx) {
eventfd_ctx_put(v->config_ctx);
+ v->config_ctx = NULL;
+ }
}
static long vhost_vdpa_set_config_call(struct vhost_vdpa *v, u32 __user *argp)
--
2.29.2
In vhost_vdpa_set_config_call() if eventfd_ctx_fdget() fails the
'v->config_ctx' contains an error instead of a valid pointer.
Since we consider 'v->config_ctx' valid if it is not NULL, we should
set it to NULL in this case to avoid to use an invalid pointer in
other functions such as vhost_vdpa_config_put().
Fixes: 776f395004d8 ("vhost_vdpa: Support config interrupt in vdpa")
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Stefano Garzarella <[email protected]>
---
drivers/vhost/vdpa.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 00796e4ecfdf..f9ecdce5468a 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -331,8 +331,12 @@ static long vhost_vdpa_set_config_call(struct vhost_vdpa *v, u32 __user *argp)
if (!IS_ERR_OR_NULL(ctx))
eventfd_ctx_put(ctx);
- if (IS_ERR(v->config_ctx))
- return PTR_ERR(v->config_ctx);
+ if (IS_ERR(v->config_ctx)) {
+ long ret = PTR_ERR(v->config_ctx);
+
+ v->config_ctx = NULL;
+ return ret;
+ }
v->vdpa->config->set_config_cb(v->vdpa, &cb);
--
2.29.2
On Thu, Mar 11, 2021 at 02:52:55PM +0100, Stefano Garzarella wrote:
> While writing a test for a Rust library [1] to handle vhost-vdpa devices,
> I experienced the 'use-after-free' issue fixed in patch 1, then I
> discovered the potential issue when eventfd_ctx_fdget() fails fixed in
> patch 2.
>
> Do you think it might be useful to write a vdpa test suite, perhaps using
> this Rust library [2] ?
> Could we put it in the kernel tree in tool/testing?
I can add tools/vdpa, no problem ...
> Thanks,
> Stefano
>
> [1] https://github.com/stefano-garzarella/vhost/tree/vdpa
> [2] https://github.com/rust-vmm/vhost
>
> Stefano Garzarella (2):
> vhost-vdpa: fix use-after-free of v->config_ctx
> vhost-vdpa: set v->config_ctx to NULL if eventfd_ctx_fdget() fails
>
> drivers/vhost/vdpa.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> --
> 2.29.2
On 3/11/2021 9:52 PM, Stefano Garzarella wrote:
> When the 'v->config_ctx' eventfd_ctx reference is released we didn't
> set it to NULL. So if the same character device (e.g. /dev/vhost-vdpa-0)
> is re-opened, the 'v->config_ctx' is invalid and calling again
> vhost_vdpa_config_put() causes use-after-free issues like the
> following refcount_t underflow:
>
> refcount_t: underflow; use-after-free.
> WARNING: CPU: 2 PID: 872 at lib/refcount.c:28 refcount_warn_saturate+0xae/0xf0
> RIP: 0010:refcount_warn_saturate+0xae/0xf0
> Call Trace:
> eventfd_ctx_put+0x5b/0x70
> vhost_vdpa_release+0xcd/0x150 [vhost_vdpa]
> __fput+0x8e/0x240
> ____fput+0xe/0x10
> task_work_run+0x66/0xa0
> exit_to_user_mode_prepare+0x118/0x120
> syscall_exit_to_user_mode+0x21/0x50
> ? __x64_sys_close+0x12/0x40
> do_syscall_64+0x45/0x50
> entry_SYSCALL_64_after_hwframe+0x44/0xae
>
> Fixes: 776f395004d8 ("vhost_vdpa: Support config interrupt in vdpa")
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Stefano Garzarella <[email protected]>
> ---
> drivers/vhost/vdpa.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index ef688c8c0e0e..00796e4ecfdf 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -308,8 +308,10 @@ static long vhost_vdpa_get_vring_num(struct vhost_vdpa *v, u16 __user *argp)
>
> static void vhost_vdpa_config_put(struct vhost_vdpa *v)
> {
> - if (v->config_ctx)
> + if (v->config_ctx) {
> eventfd_ctx_put(v->config_ctx);
> + v->config_ctx = NULL;
> + }
> }
>
> static long vhost_vdpa_set_config_call(struct vhost_vdpa *v, u32 __user *argp)
Thanks Stefano!
Reviewed-by: Zhu Lingshan <[email protected]>
On 2021/3/11 9:52 下午, Stefano Garzarella wrote:
> When the 'v->config_ctx' eventfd_ctx reference is released we didn't
> set it to NULL. So if the same character device (e.g. /dev/vhost-vdpa-0)
> is re-opened, the 'v->config_ctx' is invalid and calling again
> vhost_vdpa_config_put() causes use-after-free issues like the
> following refcount_t underflow:
>
> refcount_t: underflow; use-after-free.
> WARNING: CPU: 2 PID: 872 at lib/refcount.c:28 refcount_warn_saturate+0xae/0xf0
> RIP: 0010:refcount_warn_saturate+0xae/0xf0
> Call Trace:
> eventfd_ctx_put+0x5b/0x70
> vhost_vdpa_release+0xcd/0x150 [vhost_vdpa]
> __fput+0x8e/0x240
> ____fput+0xe/0x10
> task_work_run+0x66/0xa0
> exit_to_user_mode_prepare+0x118/0x120
> syscall_exit_to_user_mode+0x21/0x50
> ? __x64_sys_close+0x12/0x40
> do_syscall_64+0x45/0x50
> entry_SYSCALL_64_after_hwframe+0x44/0xae
>
> Fixes: 776f395004d8 ("vhost_vdpa: Support config interrupt in vdpa")
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Stefano Garzarella <[email protected]>
Acked-by: Jason Wang <[email protected]>
> ---
> drivers/vhost/vdpa.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index ef688c8c0e0e..00796e4ecfdf 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -308,8 +308,10 @@ static long vhost_vdpa_get_vring_num(struct vhost_vdpa *v, u16 __user *argp)
>
> static void vhost_vdpa_config_put(struct vhost_vdpa *v)
> {
> - if (v->config_ctx)
> + if (v->config_ctx) {
> eventfd_ctx_put(v->config_ctx);
> + v->config_ctx = NULL;
> + }
> }
>
> static long vhost_vdpa_set_config_call(struct vhost_vdpa *v, u32 __user *argp)
On 2021/3/11 9:52 下午, Stefano Garzarella wrote:
> In vhost_vdpa_set_config_call() if eventfd_ctx_fdget() fails the
> 'v->config_ctx' contains an error instead of a valid pointer.
>
> Since we consider 'v->config_ctx' valid if it is not NULL, we should
> set it to NULL in this case to avoid to use an invalid pointer in
> other functions such as vhost_vdpa_config_put().
>
> Fixes: 776f395004d8 ("vhost_vdpa: Support config interrupt in vdpa")
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Stefano Garzarella <[email protected]>
Acked-by: Jason Wang <[email protected]>
> ---
> drivers/vhost/vdpa.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index 00796e4ecfdf..f9ecdce5468a 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -331,8 +331,12 @@ static long vhost_vdpa_set_config_call(struct vhost_vdpa *v, u32 __user *argp)
> if (!IS_ERR_OR_NULL(ctx))
> eventfd_ctx_put(ctx);
>
> - if (IS_ERR(v->config_ctx))
> - return PTR_ERR(v->config_ctx);
> + if (IS_ERR(v->config_ctx)) {
> + long ret = PTR_ERR(v->config_ctx);
> +
> + v->config_ctx = NULL;
> + return ret;
> + }
>
> v->vdpa->config->set_config_cb(v->vdpa, &cb);
>