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

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

[Eric Blake]

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

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

[Peter Maydell]

On 21 April 2016 at 15:42, Eric Blake <eblake@redhat.com> 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.

I want to tag rc3 today, but since it looks like there's going to
be an rc4 for the virtio handler bug this can probably go into rc4
if it gets review.

thanks
-- PMM

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

[Kevin Wolf]

Am 21.04.2016 um 18:28 hat Peter Maydell geschrieben:
> On 21 April 2016 at 15:42, Eric Blake <eblake@redhat.com> 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.
> 
> I want to tag rc3 today, but since it looks like there's going to
> be an rc4 for the virtio handler bug this can probably go into rc4
> if it gets review.

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Peter, do you want a pull request (which I would have to do because
Paolo is away) or are you going to apply the patch directly?

Also adding Cc: qemu-stable, because this is an old bug that has existed
ever since qemu-nbd was added.

Kevin

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

[Peter Maydell]

On 22 April 2016 at 08:03, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 21.04.2016 um 18:28 hat Peter Maydell geschrieben:
>> On 21 April 2016 at 15:42, Eric Blake <eblake@redhat.com> 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.
>>
>> I want to tag rc3 today, but since it looks like there's going to
>> be an rc4 for the virtio handler bug this can probably go into rc4
>> if it gets review.
>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>
> Peter, do you want a pull request (which I would have to do because
> Paolo is away) or are you going to apply the patch directly?

If you're happy on the review and testing front I can apply it
to master directly (I won't be able to do any testing beyond
running "make check".)

thanks
-- PMM

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

[Kevin Wolf]

Am 22.04.2016 um 11:29 hat Peter Maydell geschrieben:
> On 22 April 2016 at 08:03, Kevin Wolf <kwolf@redhat.com> wrote:
> > Am 21.04.2016 um 18:28 hat Peter Maydell geschrieben:
> >> On 21 April 2016 at 15:42, Eric Blake <eblake@redhat.com> 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.
> >>
> >> I want to tag rc3 today, but since it looks like there's going to
> >> be an rc4 for the virtio handler bug this can probably go into rc4
> >> if it gets review.
> >
> > Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> >
> > Peter, do you want a pull request (which I would have to do because
> > Paolo is away) or are you going to apply the patch directly?
> 
> If you're happy on the review and testing front I can apply it
> to master directly (I won't be able to do any testing beyond
> running "make check".)

I am. It's a trivial patch anyway, but I've also tested it with
qemu-iotests and by installing a guest on an NBD device. So if you like,
you can also add:

Tested-by: Kevin Wolf <kwolf@redhat.com>

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

[Peter Maydell]

On 22 April 2016 at 11:19, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 22.04.2016 um 11:29 hat Peter Maydell geschrieben:
>> On 22 April 2016 at 08:03, Kevin Wolf <kwolf@redhat.com> wrote:
>> > Peter, do you want a pull request (which I would have to do because
>> > Paolo is away) or are you going to apply the patch directly?
>>
>> If you're happy on the review and testing front I can apply it
>> to master directly (I won't be able to do any testing beyond
>> running "make check".)
>
> I am. It's a trivial patch anyway, but I've also tested it with
> qemu-iotests and by installing a guest on an NBD device. So if you like,
> you can also add:
>
> Tested-by: Kevin Wolf <kwolf@redhat.com>

Thanks; applied to master.

-- PMM

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

[Fam Zheng]

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
> 
>