diff mbox series

[09/12] kvm-all: introduce limits for name_size and num_desc

Message ID 20230925194040.68592-10-vsementsov@yandex-team.ru (mailing list archive)
State New, archived
Headers show
Series coverity fixes | expand

Commit Message

Vladimir Sementsov-Ogievskiy Sept. 25, 2023, 7:40 p.m. UTC
Coverity doesn't like when the value with unchecked bounds that comes
from fd is used as length for IO or allocation. And really, that's not
a good practice. Let's introduce at least an empirical limits for these
values.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 accel/kvm/kvm-all.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Comments

Peter Maydell Sept. 26, 2023, 11:05 a.m. UTC | #1
On Mon, 25 Sept 2023 at 20:43, Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> Coverity doesn't like when the value with unchecked bounds that comes
> from fd is used as length for IO or allocation. And really, that's not
> a good practice. Let's introduce at least an empirical limits for these
> values.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  accel/kvm/kvm-all.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index ff1578bb32..6d0ba7d900 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -3988,6 +3988,9 @@ typedef struct StatsDescriptors {
>  static QTAILQ_HEAD(, StatsDescriptors) stats_descriptors =
>      QTAILQ_HEAD_INITIALIZER(stats_descriptors);
>
> +
> +#define KVM_STATS_QEMU_MAX_NAME_SIZE (1024 * 1024)
> +#define KVM_STATS_QEMU_MAX_NUM_DESC (1024)

These seem arbitrary. Why these values in particular?
Does the kernel have any limitation on the values it passes us?
Do we have any particular limit on what we can handle?

thanks
-- PMM
Vladimir Sementsov-Ogievskiy Sept. 26, 2023, 2:49 p.m. UTC | #2
On 26.09.23 14:05, Peter Maydell wrote:
> On Mon, 25 Sept 2023 at 20:43, Vladimir Sementsov-Ogievskiy
> <vsementsov@yandex-team.ru> wrote:
>>
>> Coverity doesn't like when the value with unchecked bounds that comes
>> from fd is used as length for IO or allocation. And really, that's not
>> a good practice. Let's introduce at least an empirical limits for these
>> values.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>   accel/kvm/kvm-all.c | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index ff1578bb32..6d0ba7d900 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -3988,6 +3988,9 @@ typedef struct StatsDescriptors {
>>   static QTAILQ_HEAD(, StatsDescriptors) stats_descriptors =
>>       QTAILQ_HEAD_INITIALIZER(stats_descriptors);
>>
>> +
>> +#define KVM_STATS_QEMU_MAX_NAME_SIZE (1024 * 1024)
>> +#define KVM_STATS_QEMU_MAX_NUM_DESC (1024)
> 
> These seem arbitrary. Why these values in particular?
> Does the kernel have any limitation on the values it passes us?

Documentation doesn't say about limits

> Do we have any particular limit on what we can handle?
> 

Hmm. At least, we don't arithmetic operations with these values to overflow. But in this case g_malloc0_n should crash anyway.

So we may rely on g_malloc0_n as on assertion that the values are good enough and further doubts are false-positives. Will drop this patch.
diff mbox series

Patch

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index ff1578bb32..6d0ba7d900 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -3988,6 +3988,9 @@  typedef struct StatsDescriptors {
 static QTAILQ_HEAD(, StatsDescriptors) stats_descriptors =
     QTAILQ_HEAD_INITIALIZER(stats_descriptors);
 
+
+#define KVM_STATS_QEMU_MAX_NAME_SIZE (1024 * 1024)
+#define KVM_STATS_QEMU_MAX_NUM_DESC (1024)
 /*
  * Return the descriptors for 'target', that either have already been read
  * or are retrieved from 'stats_fd'.
@@ -4021,6 +4024,18 @@  static StatsDescriptors *find_stats_descriptors(StatsTarget target, int stats_fd
         g_free(descriptors);
         return NULL;
     }
+    if (kvm_stats_header->name_size > KVM_STATS_QEMU_MAX_NAME_SIZE) {
+        error_setg(errp, "KVM stats: too large name_size: %" PRIu32,
+                   kvm_stats_header->name_size);
+        g_free(descriptors);
+        return NULL;
+    }
+    if (kvm_stats_header->num_desc > KVM_STATS_QEMU_MAX_NUM_DESC) {
+        error_setg(errp, "KVM stats: too large num_desc: %" PRIu32,
+                   kvm_stats_header->num_desc);
+        g_free(descriptors);
+        return NULL;
+    }
     size_desc = sizeof(*kvm_stats_desc) + kvm_stats_header->name_size;
 
     /* Read stats descriptors */