| Message ID | 20230925194040.68592-10-vsementsov@yandex-team.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | coverity fixes | expand |
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
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 --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 */
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(+)