| Message ID | 20240723150927.1396456-1-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | util/async.c: Forbid negative min/max in aio_context_set_thread_pool_params() | expand |
On 23/7/24 17:09, Peter Maydell wrote: > aio_context_set_thread_pool_params() takes two int64_t arguments to > set the minimum and maximum number of threads in the pool. We do > some bounds checking on these, but we don't catch the case where the > inputs are negative. This means that later in the function when we > assign these inputs to the AioContext::thread_pool_min and > ::thread_pool_max fields, which are of type int, the values might > overflow the smaller type. > > A negative number of threads is meaningless, so make > aio_context_set_thread_pool_params() return an error if either min or > max are negative. > > Resolves: Coverity CID 1547605 > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > util/async.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/util/async.c b/util/async.c > index 0467890052a..3e3e4fc7126 100644 > --- a/util/async.c > +++ b/util/async.c > @@ -746,7 +746,7 @@ void aio_context_set_thread_pool_params(AioContext *ctx, int64_t min, > int64_t max, Error **errp) > { > > - if (min > max || !max || min > INT_MAX || max > INT_MAX) { > + if (min > max || max <= 0 || min < 0 || min > INT_MAX || max > INT_MAX) { > error_setg(errp, "bad thread-pool-min/thread-pool-max values"); > return; > } Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> I don't get the point of using signed min/max here...
On Tue, 23 Jul 2024 at 16:44, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > On 23/7/24 17:09, Peter Maydell wrote: > > aio_context_set_thread_pool_params() takes two int64_t arguments to > > set the minimum and maximum number of threads in the pool. We do > > some bounds checking on these, but we don't catch the case where the > > inputs are negative. This means that later in the function when we > > assign these inputs to the AioContext::thread_pool_min and > > ::thread_pool_max fields, which are of type int, the values might > > overflow the smaller type. > > > > A negative number of threads is meaningless, so make > > aio_context_set_thread_pool_params() return an error if either min or > > max are negative. > > > > Resolves: Coverity CID 1547605 > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > --- > > util/async.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/util/async.c b/util/async.c > > index 0467890052a..3e3e4fc7126 100644 > > --- a/util/async.c > > +++ b/util/async.c > > @@ -746,7 +746,7 @@ void aio_context_set_thread_pool_params(AioContext *ctx, int64_t min, > > int64_t max, Error **errp) > > { > > > > - if (min > max || !max || min > INT_MAX || max > INT_MAX) { > > + if (min > max || max <= 0 || min < 0 || min > INT_MAX || max > INT_MAX) { > > error_setg(errp, "bad thread-pool-min/thread-pool-max values"); > > return; > > } > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > I don't get the point of using signed min/max here... I think this is because those values may originate in a QAPI command structure (EventLoopBaseProperties), where they are defined as "int" rather than a specifically unsigned type. So we carry them around as int64_t until they get to here, where we do the validation of whether they're sensible or not. thanks -- PMM
On 23/7/24 17:51, Peter Maydell wrote: > On Tue, 23 Jul 2024 at 16:44, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >> >> On 23/7/24 17:09, Peter Maydell wrote: >>> aio_context_set_thread_pool_params() takes two int64_t arguments to >>> set the minimum and maximum number of threads in the pool. We do >>> some bounds checking on these, but we don't catch the case where the >>> inputs are negative. This means that later in the function when we >>> assign these inputs to the AioContext::thread_pool_min and >>> ::thread_pool_max fields, which are of type int, the values might >>> overflow the smaller type. >>> >>> A negative number of threads is meaningless, so make >>> aio_context_set_thread_pool_params() return an error if either min or >>> max are negative. >>> >>> Resolves: Coverity CID 1547605 >>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >>> --- >>> util/async.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/util/async.c b/util/async.c >>> index 0467890052a..3e3e4fc7126 100644 >>> --- a/util/async.c >>> +++ b/util/async.c >>> @@ -746,7 +746,7 @@ void aio_context_set_thread_pool_params(AioContext *ctx, int64_t min, >>> int64_t max, Error **errp) >>> { >>> >>> - if (min > max || !max || min > INT_MAX || max > INT_MAX) { >>> + if (min > max || max <= 0 || min < 0 || min > INT_MAX || max > INT_MAX) { >>> error_setg(errp, "bad thread-pool-min/thread-pool-max values"); >>> return; >>> } >> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> >> I don't get the point of using signed min/max here... > > I think this is because those values may originate in a > QAPI command structure (EventLoopBaseProperties), where > they are defined as "int" rather than a specifically > unsigned type. So we carry them around as int64_t until > they get to here, where we do the validation of whether > they're sensible or not. Ah indeed. Probably very old code hard to change now (QAPI does support unsigned types).
On Tue, Jul 23, 2024 at 04:09:27PM +0100, Peter Maydell wrote: > aio_context_set_thread_pool_params() takes two int64_t arguments to > set the minimum and maximum number of threads in the pool. We do > some bounds checking on these, but we don't catch the case where the > inputs are negative. This means that later in the function when we > assign these inputs to the AioContext::thread_pool_min and > ::thread_pool_max fields, which are of type int, the values might > overflow the smaller type. > > A negative number of threads is meaningless, so make > aio_context_set_thread_pool_params() return an error if either min or > max are negative. > > Resolves: Coverity CID 1547605 > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > util/async.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Thanks, applied to my block tree: https://gitlab.com/stefanha/qemu/commits/block Stefan
diff --git a/util/async.c b/util/async.c index 0467890052a..3e3e4fc7126 100644 --- a/util/async.c +++ b/util/async.c @@ -746,7 +746,7 @@ void aio_context_set_thread_pool_params(AioContext *ctx, int64_t min, int64_t max, Error **errp) { - if (min > max || !max || min > INT_MAX || max > INT_MAX) { + if (min > max || max <= 0 || min < 0 || min > INT_MAX || max > INT_MAX) { error_setg(errp, "bad thread-pool-min/thread-pool-max values"); return; }
aio_context_set_thread_pool_params() takes two int64_t arguments to set the minimum and maximum number of threads in the pool. We do some bounds checking on these, but we don't catch the case where the inputs are negative. This means that later in the function when we assign these inputs to the AioContext::thread_pool_min and ::thread_pool_max fields, which are of type int, the values might overflow the smaller type. A negative number of threads is meaningless, so make aio_context_set_thread_pool_params() return an error if either min or max are negative. Resolves: Coverity CID 1547605 Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- util/async.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)