Re: [Qemu-devel] [PATCH for-2.6?] nbd: Don't mishandle unaligned client requests

[Fam Zheng <famz@redhat.com>]

On Thu, 04/21 08:42, Eric Blake wrote:
> The NBD protocol does not (yet) force any alignment constraints
> on clients.  Even though qemu NBD clients always send requests
> that are aligned to 512 bytes, we must be prepared for non-qemu
> clients that don't care about alignment (even if it means they
> are less efficient).  Our use of blk_read() and blk_write() was
> silently operating on the wrong file offsets when the client
> made an unaligned request, corrupting the client's data (but
> as the client already has control over the file we are serving,
> I don't think it is a security hole, per se, just a data
> corruption bug).
> 
> Note that in the case of NBD_CMD_READ, an unaligned length could
> cause us to return up to 511 bytes of uninitialized trailing
> garbage from blk_try_blockalign() - hopefully nothing sensitive
> from the heap's prior usage is ever leaked in that manner.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> 
> It's late for 2.6, but as a data corruption bug fix, I think
> it's worth having if there is still time.
> 
>  nbd/server.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index a13a691..2184c64 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1091,9 +1091,8 @@ static void nbd_trip(void *opaque)
>              }
>          }
> 
> -        ret = blk_read(exp->blk,
> -                       (request.from + exp->dev_offset) / BDRV_SECTOR_SIZE,
> -                       req->data, request.len / BDRV_SECTOR_SIZE);
> +        ret = blk_pread(exp->blk, request.from + exp->dev_offset,
> +                        req->data, request.len);
>          if (ret < 0) {
>              LOG("reading from file failed");
>              reply.error = -ret;
> @@ -1115,9 +1114,8 @@ static void nbd_trip(void *opaque)
> 
>          TRACE("Writing to device");
> 
> -        ret = blk_write(exp->blk,
> -                        (request.from + exp->dev_offset) / BDRV_SECTOR_SIZE,
> -                        req->data, request.len / BDRV_SECTOR_SIZE);
> +        ret = blk_pwrite(exp->blk, request.from + exp->dev_offset,
> +                        req->data, request.len);

Indentation is one column off, but can be ignored or fixed when applying.

Reviewed-by: Fam Zheng <famz@redhat.com>

>          if (ret < 0) {
>              LOG("writing to file failed");
>              reply.error = -ret;
> -- 
> 2.5.5
> 
>