diff mbox series

[v3,1/1] accel/kvm: Fix segmentation fault

Message ID 20240507025010.1968881-2-mii@sfc.wide.ad.jp (mailing list archive)
State New, archived
Headers show
Series [v3,1/1] accel/kvm: Fix segmentation fault | expand

Commit Message

Masato Imai May 7, 2024, 2:50 a.m. UTC
When the KVM acceleration parameter is not set, executing calc_dirty_rate
with the -r or -b option results in a segmentation fault due to accessing
a null kvm_state pointer in the kvm_dirty_ring_enabled function. This
commit adds a null check for kvm_status to prevent segmentation faults.

Signed-off-by: Masato Imai <mii@sfc.wide.ad.jp>
---
 accel/kvm/kvm-all.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Zhijian Li (Fujitsu) May 7, 2024, 3:35 a.m. UTC | #1
on 5/7/2024 10:50 AM, Masato Imai wrote:
> When the KVM acceleration parameter is not set, executing calc_dirty_rate
> with the -r or -b option results in a segmentation fault due to accessing
> a null kvm_state pointer in the kvm_dirty_ring_enabled function. This
> commit adds a null check for kvm_status to prevent segmentation faults.
>
> Signed-off-by: Masato Imai <mii@sfc.wide.ad.jp>

LGTM,
Tested-by: Li Zhijian <lizhijian@fujitsu.com>


> ---
>   accel/kvm/kvm-all.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index c0be9f5eed..544293be8a 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2329,7 +2329,7 @@ bool kvm_vcpu_id_is_valid(int vcpu_id)
>   
>   bool kvm_dirty_ring_enabled(void)
>   {
> -    return kvm_state->kvm_dirty_ring_size ? true : false;
> +    return kvm_state && kvm_state->kvm_dirty_ring_size;
>   }
>   
>   static void query_stats_cb(StatsResultList **result, StatsTarget target,
Philippe Mathieu-Daudé May 7, 2024, 7:33 a.m. UTC | #2
Hi Masato,

On 7/5/24 04:50, Masato Imai wrote:
> When the KVM acceleration parameter is not set, executing calc_dirty_rate
> with the -r or -b option results in a segmentation fault due to accessing
> a null kvm_state pointer in the kvm_dirty_ring_enabled function. This
> commit adds a null check for kvm_status to prevent segmentation faults.
> 
> Signed-off-by: Masato Imai <mii@sfc.wide.ad.jp>
> ---
>   accel/kvm/kvm-all.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index c0be9f5eed..544293be8a 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2329,7 +2329,7 @@ bool kvm_vcpu_id_is_valid(int vcpu_id)
>   
>   bool kvm_dirty_ring_enabled(void)
>   {
> -    return kvm_state->kvm_dirty_ring_size ? true : false;
> +    return kvm_state && kvm_state->kvm_dirty_ring_size;

I missed the previous iterations of this patch. I disagree
with this approach, we shouldn't call kvm_dirty_ring_enabled()
if kvm_state is NULL, this is a bad API usage. So I'd rather
assert(kvm_state) here and force the callers to check for
kvm_enabled() before calling.

>   }
>   
>   static void query_stats_cb(StatsResultList **result, StatsTarget target,
diff mbox series

Patch

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index c0be9f5eed..544293be8a 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2329,7 +2329,7 @@  bool kvm_vcpu_id_is_valid(int vcpu_id)
 
 bool kvm_dirty_ring_enabled(void)
 {
-    return kvm_state->kvm_dirty_ring_size ? true : false;
+    return kvm_state && kvm_state->kvm_dirty_ring_size;
 }
 
 static void query_stats_cb(StatsResultList **result, StatsTarget target,