| Message ID | 20211203231539.3900865-5-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qemu patches for NBD_OPT_EXTENDED_HEADERS | expand |
04.12.2021 02:15, Eric Blake wrote: > Our existing use of structured replies either reads into a qiov capped > at 32M (NBD_CMD_READ) or caps allocation to 1000 bytes (see > NBD_MAX_MALLOC_PAYLOAD in block/nbd.c). But the existing length > checks are rather late; if we encounter a buggy (or malicious) server > that sends a super-large payload length, we should drop the connection > right then rather than assuming the layer on top will be careful. > This becomes more important when we permit 64-bit lengths which are > even more likely to have the potential for attempted denial of service > abuse. > > Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > nbd/client.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/nbd/client.c b/nbd/client.c > index 30d5383cb195..8f137c2320bb 100644 > --- a/nbd/client.c > +++ b/nbd/client.c > @@ -1412,6 +1412,18 @@ static int nbd_receive_structured_reply_chunk(QIOChannel *ioc, > chunk->handle = be64_to_cpu(chunk->handle); > chunk->length = be32_to_cpu(chunk->length); > > + /* > + * Because we use BLOCK_STATUS with REQ_ONE, and cap READ requests > + * at 32M, no valid server should send us payload larger than > + * this. Even if we stopped using REQ_ONE, sane servers will cap > + * the number of extents they return for block status. > + */ > + if (chunk->length > NBD_MAX_BUFFER_SIZE + sizeof(NBDStructuredReadData)) { > + error_setg(errp, "server chunk %" PRIu32 " (%s) payload is too long", > + chunk->type, nbd_rep_lookup(chunk->type)); > + return -EINVAL; > + } > + > return 0; > } >
diff --git a/nbd/client.c b/nbd/client.c index 30d5383cb195..8f137c2320bb 100644 --- a/nbd/client.c +++ b/nbd/client.c @@ -1412,6 +1412,18 @@ static int nbd_receive_structured_reply_chunk(QIOChannel *ioc, chunk->handle = be64_to_cpu(chunk->handle); chunk->length = be32_to_cpu(chunk->length); + /* + * Because we use BLOCK_STATUS with REQ_ONE, and cap READ requests + * at 32M, no valid server should send us payload larger than + * this. Even if we stopped using REQ_ONE, sane servers will cap + * the number of extents they return for block status. + */ + if (chunk->length > NBD_MAX_BUFFER_SIZE + sizeof(NBDStructuredReadData)) { + error_setg(errp, "server chunk %" PRIu32 " (%s) payload is too long", + chunk->type, nbd_rep_lookup(chunk->type)); + return -EINVAL; + } + return 0; }
Our existing use of structured replies either reads into a qiov capped at 32M (NBD_CMD_READ) or caps allocation to 1000 bytes (see NBD_MAX_MALLOC_PAYLOAD in block/nbd.c). But the existing length checks are rather late; if we encounter a buggy (or malicious) server that sends a super-large payload length, we should drop the connection right then rather than assuming the layer on top will be careful. This becomes more important when we permit 64-bit lengths which are even more likely to have the potential for attempted denial of service abuse. Signed-off-by: Eric Blake <eblake@redhat.com> --- nbd/client.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)