On 20.01.20 13:28, Vladimir Sementsov-Ogievskiy wrote: > 20.01.2020 14:59, Max Reitz wrote: >> On 19.12.19 11:03, Vladimir Sementsov-Ogievskiy wrote: >>> We are going to introduce bdrv_dirty_bitmap_next_dirty so that same >>> variable may be used to store its return value and to be its parameter, >>> so it would int64_t. >>> >>> Similarly, we are going to refactor hbitmap_next_dirty_area to use >>> hbitmap_next_dirty together with hbitmap_next_zero, therefore we want >>> hbitmap_next_zero parameter type to be int64_t too. >>> >>> So, for convenience update all parameters of *_next_zero and >>> *_next_dirty_area to be int64_t. >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <[email protected]> >>> --- >>> include/block/dirty-bitmap.h | 6 +++--- >>> include/qemu/hbitmap.h | 7 +++---- >>> block/dirty-bitmap.c | 6 +++--- >>> nbd/server.c | 2 +- >>> tests/test-hbitmap.c | 32 ++++++++++++++++---------------- >>> util/hbitmap.c | 13 ++++++++----- >>> 6 files changed, 34 insertions(+), 32 deletions(-) >> >> [...] >> >>> diff --git a/util/hbitmap.c b/util/hbitmap.c >>> index b6d4b99a06..df22f06be6 100644 >>> --- a/util/hbitmap.c >>> +++ b/util/hbitmap.c >>> @@ -193,7 +193,7 @@ void hbitmap_iter_init(HBitmapIter *hbi, const HBitmap >>> *hb, uint64_t first) >>> } >>> } >>> >>> -int64_t hbitmap_next_zero(const HBitmap *hb, uint64_t start, uint64_t >>> count) >>> +int64_t hbitmap_next_zero(const HBitmap *hb, int64_t start, int64_t count) >>> { >>> size_t pos = (start >> hb->granularity) >> BITS_PER_LEVEL; >>> unsigned long *last_lev = hb->levels[HBITMAP_LEVELS - 1]; >>> @@ -202,6 +202,8 @@ int64_t hbitmap_next_zero(const HBitmap *hb, uint64_t >>> start, uint64_t count) >>> uint64_t end_bit, sz; >>> int64_t res; >>> >>> + assert(start >= 0 && count >= 0); >>> + >>> if (start >= hb->orig_size || count == 0) { >>> return -1; >>> } >> As far as I can see, NBD just passes NBDRequest.from (which is a >> uint64_t) to this function (on NBD_CMD_BLOCK_STATUS). Would this allow >> a malicious client to send a value > INT64_MAX, thus provoking an >> overflow and killing the server with this new assertion? > > > in nbd_co_receive_request() we have > > > if (request->from > client->exp->size || > request->len > client->exp->size - request->from) { > > > So, we check that from is <= exp->size. and exp->size cant be greater than > INT64_MAX, > as it derived from bdrv_getlength, which returns int64_t.
Ah, OK, so I just overlooked that.
> Interesting, should we be more strict in server:?
>
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -2178,7 +2178,7 @@ static int nbd_co_receive_request(NBDRequestData *req,
> NBDRequest *request,
> error_setg(errp, "Export is read-only");
> return -EROFS;
> }
> - if (request->from > client->exp->size ||
> + if (request->from >= client->exp->size ||
> request->len > client->exp->size - request->from) {
> error_setg(errp, "operation past EOF; From: %" PRIu64 ", Len: %"
> PRIu32
> ", Size: %" PRIu64, request->from, request->len,
>
> Or is it intentional? Looking through NBD spec I found only
>
> client MUST NOT use a length ... or which, when added to offset, would
> exceed the export size.
>
> So, formally pair offset=<export size>, len=0 is valid...
Sounds valid, yes.
In any case:
Reviewed-by: Max Reitz <[email protected]>
signature.asc
Description: OpenPGP digital signature
