Message ID | 20171115213557.3548-1-eblake@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
16.11.2017 00:35, Eric Blake wrote: > The NBD spec says an attempt to NBD_CMD_TRIM on a read-only > export should fail with EPERM, as a trim has the potential > to change disk contents, but we were relying on the block > layer to catch that for us, which might not always give the > right error (and even if it does, it does not let us pass > back a sane message for structured replies). > > The NBD spec says an attempt to NBD_CMD_WRITE_ZEROES out of > bounds should fail with ENOSPC, not EINVAL. > > Our check for u64 offset + u32 length wraparound up front is > pointless; nothing uses offset until after the second round > of sanity checks, and we can just as easily ensure there is > no wraparound by checking whether offset is in bounds (since > a disk size cannot exceed off_t which is 63 bits, adding a > 32-bit number for a valid offset can't overflow). looks like here is another problem which you've fixed: with old code connection would be lost if this check fails in case of CMD_WRITE, as req->complete would be false. > > Solve all of these issues by some code motion and improved > request validation. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > v2: actually commit the compiler-error fixes before submitting... > > nbd/server.c | 36 ++++++++++++------------------------ > 1 file changed, 12 insertions(+), 24 deletions(-) > > diff --git a/nbd/server.c b/nbd/server.c > index df771fd42f..7d6801b427 100644 > --- a/nbd/server.c > +++ b/nbd/server.c > @@ -1366,15 +1366,6 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request, > return -EIO; > } > > - /* Check for sanity in the parameters, part 1. Defer as many > - * checks as possible until after reading any NBD_CMD_WRITE > - * payload, so we can try and keep the connection alive. */ I don't understand the comment. Opposite: to keep connection alive we must read the payload, even in case of sanity check fail. > - if ((request->from + request->len) < request->from) { > - error_setg(errp, > - "integer overflow detected, you're probably being attacked"); > - return -EINVAL; > - } > - > if (request->type == NBD_CMD_READ || request->type == NBD_CMD_WRITE) { > if (request->len > NBD_MAX_BUFFER_SIZE) { > error_setg(errp, "len (%" PRIu32" ) is larger than max len (%u)", related idea here: if request->len > NBD_MAX_BUFFER_SIZE or if we failed to allocate buffer in following if, we can call nbd_drop to read CMD_WRITE payload and set req->complete = true;, to keep connection in this cases. However, it may be done later. > @@ -1399,12 +1390,21 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request, > request->len); > } > > - /* Sanity checks, part 2. */ > - if (request->from + request->len > client->exp->size) { > + /* Sanity checks. */ > + if (client->exp->nbdflags & NBD_FLAG_READ_ONLY && > + (request->type == NBD_CMD_WRITE || > + request->type == NBD_CMD_WRITE_ZEROES || > + request->type == NBD_CMD_TRIM)) { > + error_setg(errp, "Export is read-only"); > + return -EROFS; > + } > + if (request->from > client->exp->size || > + request->from + request->len > client->exp->size) { > error_setg(errp, "operation past EOF; From: %" PRIu64 ", Len: %" PRIu32 > ", Size: %" PRIu64, request->from, request->len, > (uint64_t)client->exp->size); > - return request->type == NBD_CMD_WRITE ? -ENOSPC : -EINVAL; > + return (request->type == NBD_CMD_WRITE || > + request->type == NBD_CMD_WRITE_ZEROES) ? -ENOSPC : -EINVAL; > } > valid_flags = NBD_CMD_FLAG_FUA; > if (request->type == NBD_CMD_READ && client->structured_reply) { > @@ -1482,12 +1482,6 @@ static coroutine_fn void nbd_trip(void *opaque) > > break; > case NBD_CMD_WRITE: > - if (exp->nbdflags & NBD_FLAG_READ_ONLY) { > - error_setg(&local_err, "Export is read-only"); > - ret = -EROFS; > - break; > - } > - > flags = 0; > if (request.flags & NBD_CMD_FLAG_FUA) { > flags |= BDRV_REQ_FUA; > @@ -1500,12 +1494,6 @@ static coroutine_fn void nbd_trip(void *opaque) > > break; > case NBD_CMD_WRITE_ZEROES: > - if (exp->nbdflags & NBD_FLAG_READ_ONLY) { > - error_setg(&local_err, "Export is read-only"); > - ret = -EROFS; > - break; > - } > - > flags = 0; > if (request.flags & NBD_CMD_FLAG_FUA) { > flags |= BDRV_REQ_FUA; Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
On 11/16/2017 02:52 AM, Vladimir Sementsov-Ogievskiy wrote: >> if (request->type == NBD_CMD_READ || request->type == >> NBD_CMD_WRITE) { >> if (request->len > NBD_MAX_BUFFER_SIZE) { >> error_setg(errp, "len (%" PRIu32" ) is larger than max >> len (%u)", > > related idea here: if request->len > NBD_MAX_BUFFER_SIZE or if we failed > to allocate buffer in following if, > we can call nbd_drop to read CMD_WRITE payload and set req->complete = > true;, to keep connection in this > cases. > > However, it may be done later. On the other hand, if request->len is too huge (it can be up to 4G, where we only want 32M at the most), then we really don't want to waste time on the nbd_drop() call. That's why req->complete exists in the first place. You are right that we could nbd_drop() a padded size (for example, I just patched nbdkit to gracefully skip up to twice the maximum block size, and only drop the connection when it exceeds NBD_MAX_BUFFER_SIZE*2), but again, it's all in the quality-of-implementation area (a client sending that much data is already in denial-of-service territory, so we are okay dropping the connection). So I'm not worried about any further tweaks to this code for 2.11. > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Thanks; this patch is now on my queue for 2.11-rc2.
diff --git a/nbd/server.c b/nbd/server.c index df771fd42f..7d6801b427 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -1366,15 +1366,6 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request, return -EIO; } - /* Check for sanity in the parameters, part 1. Defer as many - * checks as possible until after reading any NBD_CMD_WRITE - * payload, so we can try and keep the connection alive. */ - if ((request->from + request->len) < request->from) { - error_setg(errp, - "integer overflow detected, you're probably being attacked"); - return -EINVAL; - } - if (request->type == NBD_CMD_READ || request->type == NBD_CMD_WRITE) { if (request->len > NBD_MAX_BUFFER_SIZE) { error_setg(errp, "len (%" PRIu32" ) is larger than max len (%u)", @@ -1399,12 +1390,21 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request, request->len); } - /* Sanity checks, part 2. */ - if (request->from + request->len > client->exp->size) { + /* Sanity checks. */ + if (client->exp->nbdflags & NBD_FLAG_READ_ONLY && + (request->type == NBD_CMD_WRITE || + request->type == NBD_CMD_WRITE_ZEROES || + request->type == NBD_CMD_TRIM)) { + error_setg(errp, "Export is read-only"); + return -EROFS; + } + if (request->from > client->exp->size || + request->from + request->len > client->exp->size) { error_setg(errp, "operation past EOF; From: %" PRIu64 ", Len: %" PRIu32 ", Size: %" PRIu64, request->from, request->len, (uint64_t)client->exp->size); - return request->type == NBD_CMD_WRITE ? -ENOSPC : -EINVAL; + return (request->type == NBD_CMD_WRITE || + request->type == NBD_CMD_WRITE_ZEROES) ? -ENOSPC : -EINVAL; } valid_flags = NBD_CMD_FLAG_FUA; if (request->type == NBD_CMD_READ && client->structured_reply) { @@ -1482,12 +1482,6 @@ static coroutine_fn void nbd_trip(void *opaque) break; case NBD_CMD_WRITE: - if (exp->nbdflags & NBD_FLAG_READ_ONLY) { - error_setg(&local_err, "Export is read-only"); - ret = -EROFS; - break; - } - flags = 0; if (request.flags & NBD_CMD_FLAG_FUA) { flags |= BDRV_REQ_FUA; @@ -1500,12 +1494,6 @@ static coroutine_fn void nbd_trip(void *opaque) break; case NBD_CMD_WRITE_ZEROES: - if (exp->nbdflags & NBD_FLAG_READ_ONLY) { - error_setg(&local_err, "Export is read-only"); - ret = -EROFS; - break; - } - flags = 0; if (request.flags & NBD_CMD_FLAG_FUA) { flags |= BDRV_REQ_FUA;
The NBD spec says an attempt to NBD_CMD_TRIM on a read-only export should fail with EPERM, as a trim has the potential to change disk contents, but we were relying on the block layer to catch that for us, which might not always give the right error (and even if it does, it does not let us pass back a sane message for structured replies). The NBD spec says an attempt to NBD_CMD_WRITE_ZEROES out of bounds should fail with ENOSPC, not EINVAL. Our check for u64 offset + u32 length wraparound up front is pointless; nothing uses offset until after the second round of sanity checks, and we can just as easily ensure there is no wraparound by checking whether offset is in bounds (since a disk size cannot exceed off_t which is 63 bits, adding a 32-bit number for a valid offset can't overflow). Solve all of these issues by some code motion and improved request validation. Signed-off-by: Eric Blake <eblake@redhat.com> --- v2: actually commit the compiler-error fixes before submitting... nbd/server.c | 36 ++++++++++++------------------------ 1 file changed, 12 insertions(+), 24 deletions(-)