| Message ID | 20160920114103.27aeea2a@fiorina |
|---|---|
| State | New |
| Headers | show |
On 20/09/2016 11:41, Tomáš Golembiovský wrote: > When image is part of the file it makes sense to limit the length of the > image in the file. Otherwise it is assumed that the image spans to the > end of the file. This assumption may lead to reads/writes outside of the > image and thus lead to errors or data corruption. > > To limit the assumed image size new option is introduced. The patch makes sense, but I think the commit message is incorrect because this bug is already fixed by patch 1. Also, the option in the help is --device-size, not --image-size; I would just call it --size. Thanks, Paolo > > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> > --- > qemu-nbd.c | 44 +++++++++++++++++++++++++++++++++++--------- > qemu-nbd.texi | 4 ++++ > 2 files changed, 39 insertions(+), 9 deletions(-) > > diff --git a/qemu-nbd.c b/qemu-nbd.c > index 629bce1..7ed52f7 100644 > --- a/qemu-nbd.c > +++ b/qemu-nbd.c > @@ -85,6 +85,7 @@ static void usage(const char *name) > "\n" > "Exposing part of the image:\n" > " -o, --offset=OFFSET offset into the image\n" > +" -S, --device-size=LEN limit reported device size\n" > " -P, --partition=NUM only expose partition NUM\n" > "\n" > "General purpose options:\n" > @@ -471,10 +472,12 @@ int main(int argc, char **argv) > const char *port = NULL; > char *sockpath = NULL; > char *device = NULL; > - off_t fd_size; > + off_t real_size = 0; > + off_t fd_size = 0; > + bool has_image_size = false; > QemuOpts *sn_opts = NULL; > const char *sn_id_or_name = NULL; > - const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:T:"; > + const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:T:S:"; > struct option lopt[] = { > { "help", no_argument, NULL, 'h' }, > { "version", no_argument, NULL, 'V' }, > @@ -482,6 +485,7 @@ int main(int argc, char **argv) > { "port", required_argument, NULL, 'p' }, > { "socket", required_argument, NULL, 'k' }, > { "offset", required_argument, NULL, 'o' }, > + { "image-size", required_argument, NULL, 'S' }, > { "read-only", no_argument, NULL, 'r' }, > { "partition", required_argument, NULL, 'P' }, > { "connect", required_argument, NULL, 'c' }, > @@ -714,6 +718,18 @@ int main(int argc, char **argv) > g_free(trace_file); > trace_file = trace_opt_parse(optarg); > break; > + case 'S': > + ret = qemu_strtoll(optarg, NULL, 0, &fd_size); > + if (ret != 0) { > + error_report("Invalid image size `%s'", optarg); > + exit(EXIT_FAILURE); > + } > + if (fd_size <= 0) { > + error_report("Image size must be positive `%s'", optarg); > + exit(EXIT_FAILURE); > + } > + has_image_size = true; > + break; > } > } > > @@ -894,19 +910,29 @@ int main(int argc, char **argv) > } > > bs->detect_zeroes = detect_zeroes; > - fd_size = blk_getlength(blk); > - if (fd_size < 0) { > + real_size = blk_getlength(blk); > + if (real_size < 0) { > error_report("Failed to determine the image length: %s", > - strerror(-fd_size)); > + strerror(-real_size)); > exit(EXIT_FAILURE); > } > > - if (dev_offset >= fd_size) { > - error_report("Offset (%lu) has to be smaller than the image size (%lu)", > - dev_offset, fd_size); > + if (!has_image_size) { > + fd_size = real_size; > + > + if (dev_offset >= fd_size) { > + error_report("Offset (%lu) has to be smaller than image size (%lu)", > + dev_offset, fd_size); > + exit(EXIT_FAILURE); > + } > + > + fd_size -= dev_offset; > + } else if ((dev_offset + fd_size) > real_size) { > + error_report("Offset (%lu) plus image size (%lu) has to be smaller " > + "than or equal to real image size (%lu)", > + dev_offset, fd_size, real_size); > exit(EXIT_FAILURE); > } > - fd_size -= dev_offset; > > if (partition != -1) { > ret = find_partition(blk, partition, &dev_offset, &fd_size); > diff --git a/qemu-nbd.texi b/qemu-nbd.texi > index 91ebf04..c589525 100644 > --- a/qemu-nbd.texi > +++ b/qemu-nbd.texi > @@ -30,6 +30,10 @@ credentials for the qemu-nbd server. > The TCP port to listen on (default @samp{10809}) > @item -o, --offset=@var{offset} > The offset into the image > +@item -S, --image-size=@var{length} > +The size of the image to present to client. This is useful in combination with > +@var{-o} when the image is embedded in file but does not span to the end of the > +file. > @item -b, --bind=@var{iface} > The interface to bind to (default @samp{0.0.0.0}) > @item -k, --socket=@var{path} >
On Tue, Sep 20, 2016 at 11:59:28AM +0200, Paolo Bonzini wrote: > > > On 20/09/2016 11:41, Tomáš Golembiovský wrote: > > When image is part of the file it makes sense to limit the length of the > > image in the file. Otherwise it is assumed that the image spans to the > > end of the file. This assumption may lead to reads/writes outside of the > > image and thus lead to errors or data corruption. > > > > To limit the assumed image size new option is introduced. > > The patch makes sense, but I think the commit message is incorrect > because this bug is already fixed by patch 1. Also, the option in the > help is --device-size, not --image-size; I would just call it --size. I don't think it makes sense as a special case in qemu-nbd. It feels like this is better done by extending the 'raw' block driver with 'offset' and 'length' parameters. You can then layer the 'raw' block driver over any existing QEMU blocker driver, anywhere in QEMU / tools that accept a block device description. No need to add extra parameters to any of the programs. Regards, Daniel
On Tue, 20 Sep 2016 11:59:28 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 20/09/2016 11:41, Tomáš Golembiovský wrote: > > When image is part of the file it makes sense to limit the length of the > > image in the file. Otherwise it is assumed that the image spans to the > > end of the file. This assumption may lead to reads/writes outside of the > > image and thus lead to errors or data corruption. > > > > To limit the assumed image size new option is introduced. > > The patch makes sense, but I think the commit message is incorrect > because this bug is already fixed by patch 1. Also, the option in the I agree that the wording is not completely clear. The patches solve two different but related problems. The first patch solves situation where you have file containing: || some data ; image || In this case qemu-nbd tried to access data outside the file. But there is nothing there, because the file is shorter. The second patch tries to solve the situation when you have file: || some data ; image ; some more data || In this case there is no way to say where the image ends and client may also access content in the "some more data" area. Thus corrupting the data outside the image. What about something like this: Normally qemu-nbd assumes that the image spans from the beginning of the file (or from position specified by --offset) to the end of the file. If the image is embedded inside the file and there are some other data after the image this may lead to reads/writes outside the image and data corruption. This patch adds new command line argument --size that limits the assumed device size. This way the user can specify that the image ends sooner than at the end of the file. > help is --device-size, not --image-size; I would just call it --size. Ok. I will change it to --size. > > Thanks, > > Paolo >
On 20/09/2016 13:12, Daniel P. Berrange wrote: > On Tue, Sep 20, 2016 at 11:59:28AM +0200, Paolo Bonzini wrote: >> >> >> On 20/09/2016 11:41, Tomáš Golembiovský wrote: >>> When image is part of the file it makes sense to limit the length of the >>> image in the file. Otherwise it is assumed that the image spans to the >>> end of the file. This assumption may lead to reads/writes outside of the >>> image and thus lead to errors or data corruption. >>> >>> To limit the assumed image size new option is introduced. >> >> The patch makes sense, but I think the commit message is incorrect >> because this bug is already fixed by patch 1. Also, the option in the >> help is --device-size, not --image-size; I would just call it --size. > > I don't think it makes sense as a special case in qemu-nbd. > > It feels like this is better done by extending the 'raw' > block driver with 'offset' and 'length' parameters. You > can then layer the 'raw' block driver over any existing > QEMU blocker driver, anywhere in QEMU / tools that accept > a block device description. No need to add extra parameters > to any of the programs. This makes sense too (and then --offset and partitions can be implemented on top). Paolo
On Tue, Sep 20, 2016 at 01:35:29PM +0200, Tomáš Golembiovský wrote: > The second patch tries to solve the situation when you have file: > > || some data ; image ; some more data || > > In this case there is no way to say where the image ends and client may > also access content in the "some more data" area. Thus corrupting the > data outside the image. Some background: A use case for this is to be able to access a disk image which is located inside an OVA file, without unpacking the OVA. An OVA file is [usually, not always] an uncompressed tar file. It is relatively easy to find the offset and size of the contained disk image, since tar files are essentially concatenations of header + data + header + ... OVA files may be terabytes in size, so avoiding unpacking is desirable since it can save masses of disk space and time. One place we could use this feature would be in virt-v2v. Rich.
On Tue, 20 Sep 2016 13:45:08 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 20/09/2016 13:12, Daniel P. Berrange wrote: > > On Tue, Sep 20, 2016 at 11:59:28AM +0200, Paolo Bonzini wrote: > >> > >> > >> On 20/09/2016 11:41, Tomáš Golembiovský wrote: > >>> When image is part of the file it makes sense to limit the length of the > >>> image in the file. Otherwise it is assumed that the image spans to the > >>> end of the file. This assumption may lead to reads/writes outside of the > >>> image and thus lead to errors or data corruption. > >>> > >>> To limit the assumed image size new option is introduced. > >> > >> The patch makes sense, but I think the commit message is incorrect > >> because this bug is already fixed by patch 1. Also, the option in the > >> help is --device-size, not --image-size; I would just call it --size. > > > > I don't think it makes sense as a special case in qemu-nbd. > > > > It feels like this is better done by extending the 'raw' > > block driver with 'offset' and 'length' parameters. You > > can then layer the 'raw' block driver over any existing > > QEMU blocker driver, anywhere in QEMU / tools that accept > > a block device description. No need to add extra parameters > > to any of the programs. > > This makes sense too (and then --offset and partitions can be > implemented on top). OK. Let's drop this change then. I have sent an initial attempt to implement this in raw block driver here [1]. The change only covers files opened in read-only mode for the moment so it will take a little bit more effort before it can be used in qemu-nbd. That being said, I believe the first [2] part of the series still should be considered for merging. Unless there is something intrinsically wrong with the patch. Once the necessary stuff is in the raw driver we can change the related code in qemu-nbd. Regards, Tomas [1] https://lists.nongnu.org/archive/html/qemu-block/2016-10/msg00008.html [2] https://lists.nongnu.org/archive/html/qemu-devel/2016-09/msg04554.html
diff --git a/qemu-nbd.c b/qemu-nbd.c index 629bce1..7ed52f7 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -85,6 +85,7 @@ static void usage(const char *name) "\n" "Exposing part of the image:\n" " -o, --offset=OFFSET offset into the image\n" +" -S, --device-size=LEN limit reported device size\n" " -P, --partition=NUM only expose partition NUM\n" "\n" "General purpose options:\n" @@ -471,10 +472,12 @@ int main(int argc, char **argv) const char *port = NULL; char *sockpath = NULL; char *device = NULL; - off_t fd_size; + off_t real_size = 0; + off_t fd_size = 0; + bool has_image_size = false; QemuOpts *sn_opts = NULL; const char *sn_id_or_name = NULL; - const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:T:"; + const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:T:S:"; struct option lopt[] = { { "help", no_argument, NULL, 'h' }, { "version", no_argument, NULL, 'V' }, @@ -482,6 +485,7 @@ int main(int argc, char **argv) { "port", required_argument, NULL, 'p' }, { "socket", required_argument, NULL, 'k' }, { "offset", required_argument, NULL, 'o' }, + { "image-size", required_argument, NULL, 'S' }, { "read-only", no_argument, NULL, 'r' }, { "partition", required_argument, NULL, 'P' }, { "connect", required_argument, NULL, 'c' }, @@ -714,6 +718,18 @@ int main(int argc, char **argv) g_free(trace_file); trace_file = trace_opt_parse(optarg); break; + case 'S': + ret = qemu_strtoll(optarg, NULL, 0, &fd_size); + if (ret != 0) { + error_report("Invalid image size `%s'", optarg); + exit(EXIT_FAILURE); + } + if (fd_size <= 0) { + error_report("Image size must be positive `%s'", optarg); + exit(EXIT_FAILURE); + } + has_image_size = true; + break; } } @@ -894,19 +910,29 @@ int main(int argc, char **argv) } bs->detect_zeroes = detect_zeroes; - fd_size = blk_getlength(blk); - if (fd_size < 0) { + real_size = blk_getlength(blk); + if (real_size < 0) { error_report("Failed to determine the image length: %s", - strerror(-fd_size)); + strerror(-real_size)); exit(EXIT_FAILURE); } - if (dev_offset >= fd_size) { - error_report("Offset (%lu) has to be smaller than the image size (%lu)", - dev_offset, fd_size); + if (!has_image_size) { + fd_size = real_size; + + if (dev_offset >= fd_size) { + error_report("Offset (%lu) has to be smaller than image size (%lu)", + dev_offset, fd_size); + exit(EXIT_FAILURE); + } + + fd_size -= dev_offset; + } else if ((dev_offset + fd_size) > real_size) { + error_report("Offset (%lu) plus image size (%lu) has to be smaller " + "than or equal to real image size (%lu)", + dev_offset, fd_size, real_size); exit(EXIT_FAILURE); } - fd_size -= dev_offset; if (partition != -1) { ret = find_partition(blk, partition, &dev_offset, &fd_size); diff --git a/qemu-nbd.texi b/qemu-nbd.texi index 91ebf04..c589525 100644 --- a/qemu-nbd.texi +++ b/qemu-nbd.texi @@ -30,6 +30,10 @@ credentials for the qemu-nbd server. The TCP port to listen on (default @samp{10809}) @item -o, --offset=@var{offset} The offset into the image +@item -S, --image-size=@var{length} +The size of the image to present to client. This is useful in combination with +@var{-o} when the image is embedded in file but does not span to the end of the +file. @item -b, --bind=@var{iface} The interface to bind to (default @samp{0.0.0.0}) @item -k, --socket=@var{path}
When image is part of the file it makes sense to limit the length of the image in the file. Otherwise it is assumed that the image spans to the end of the file. This assumption may lead to reads/writes outside of the image and thus lead to errors or data corruption. To limit the assumed image size new option is introduced. Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com> --- qemu-nbd.c | 44 +++++++++++++++++++++++++++++++++++--------- qemu-nbd.texi | 4 ++++ 2 files changed, 39 insertions(+), 9 deletions(-)