| Message ID | 20181215135324.152629-8-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nbd: add qemu-nbd --list | expand |
On Sat, Dec 15, 2018 at 07:53:09AM -0600, Eric Blake wrote: > Our copy-and-pasted open-coding of strtol handling forgot to > handle overflow conditions. Use qemu_strto*() instead. > > In the case of --partition, since we insist on a user-supplied > partition to be non-zero, we can use 0 rather than -1 for our > initial value to distinguish when a partition is not being > served, for slightly more optimal code. > > The error messages for out-of-bounds values are less specific, > but should not be a terrible loss in quality. > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v2: Retitle, catch more uses of strtol > [Hmm - this depends on int64_t and off_t being compatible; if they > aren't that way on all platforms, I'll need a temporary variable] > --- > qemu-nbd.c | 33 ++++++++++----------------------- > 1 file changed, 10 insertions(+), 23 deletions(-) > > diff --git a/qemu-nbd.c b/qemu-nbd.c > index 2807e132396..e3f739671b5 100644 > --- a/qemu-nbd.c > +++ b/qemu-nbd.c > @@ -544,9 +544,8 @@ int main(int argc, char **argv) > }; > int ch; > int opt_ind = 0; > - char *end; > int flags = BDRV_O_RDWR; > - int partition = -1; > + int partition = 0; > int ret = 0; > bool seen_cache = false; > bool seen_discard = false; > @@ -657,13 +656,9 @@ int main(int argc, char **argv) > port = optarg; > break; > case 'o': > - dev_offset = strtoll (optarg, &end, 0); > - if (*end) { > - error_report("Invalid offset `%s'", optarg); > - exit(EXIT_FAILURE); > - } > - if (dev_offset < 0) { > - error_report("Offset must be positive `%s'", optarg); > + if (qemu_strtoi64(optarg, NULL, 0, &dev_offset) < 0 || > + dev_offset < 0) { > + error_report("Invalid offset '%s'", optarg); > exit(EXIT_FAILURE); > } > break; > @@ -685,13 +680,9 @@ int main(int argc, char **argv) > flags &= ~BDRV_O_RDWR; > break; > case 'P': > - partition = strtol(optarg, &end, 0); > - if (*end) { > - error_report("Invalid partition `%s'", optarg); > - exit(EXIT_FAILURE); > - } > - if (partition < 1 || partition > 8) { > - error_report("Invalid partition %d", partition); > + if (qemu_strtoi(optarg, NULL, 0, &partition) < 0 || > + partition < 1 || partition > 8) { > + error_report("Invalid partition '%s'", optarg); > exit(EXIT_FAILURE); > } > break; > @@ -709,15 +700,11 @@ int main(int argc, char **argv) > device = optarg; > break; > case 'e': > - shared = strtol(optarg, &end, 0); > - if (*end) { > + if (qemu_strtoi(optarg, NULL, 0, &shared) < 0 || > + shared < 1) { > error_report("Invalid shared device number '%s'", optarg); > exit(EXIT_FAILURE); > } > - if (shared < 1) { > - error_report("Shared device number must be greater than 0"); > - exit(EXIT_FAILURE); > - } > break; > case 'f': > fmt = optarg; > @@ -1006,7 +993,7 @@ int main(int argc, char **argv) > } > fd_size -= dev_offset; > > - if (partition != -1) { > + if (partition) { > ret = find_partition(blk, partition, &dev_offset, &fd_size); > if (ret < 0) { > error_report("Could not find partition %d: %s", partition, Using qemu_strtoi has made the patch slightly bigger that last time but is still a simplification, so: Reviewed-by: Richard W.M. Jones <rjones@redhat.com> Rich.
15.12.2018 16:53, Eric Blake wrote: > Our copy-and-pasted open-coding of strtol handling forgot to > handle overflow conditions. Use qemu_strto*() instead. > > In the case of --partition, since we insist on a user-supplied > partition to be non-zero, we can use 0 rather than -1 for our > initial value to distinguish when a partition is not being > served, for slightly more optimal code. > > The error messages for out-of-bounds values are less specific, > but should not be a terrible loss in quality. > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v2: Retitle, catch more uses of strtol > [Hmm - this depends on int64_t and off_t being compatible; if they > aren't that way on all platforms, I'll need a temporary variable] hmm, as I understand, even if this compatibility exists, it's not a part of standard and nothing about off_t size in POSIX.. Moreover: what is the reason for using off_t in NBD code? We don't have it in NBD protocol, we don't have it in generic block layer interface. Isn't it always casted to int64_t or like this?
On 12/18/18 9:11 AM, Vladimir Sementsov-Ogievskiy wrote: > 15.12.2018 16:53, Eric Blake wrote: >> Our copy-and-pasted open-coding of strtol handling forgot to >> handle overflow conditions. Use qemu_strto*() instead. >> >> In the case of --partition, since we insist on a user-supplied >> partition to be non-zero, we can use 0 rather than -1 for our >> initial value to distinguish when a partition is not being >> served, for slightly more optimal code. >> >> The error messages for out-of-bounds values are less specific, >> but should not be a terrible loss in quality. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> >> --- >> v2: Retitle, catch more uses of strtol >> [Hmm - this depends on int64_t and off_t being compatible; if they >> aren't that way on all platforms, I'll need a temporary variable] > > hmm, as I understand, even if this compatibility exists, it's not a part > of standard and nothing about off_t size in POSIX.. off_t allows you to run on older systems with 32-bit offsets and newer systems with 64-bit offsets; but these days, even on 32-bit systems, we compile qemu to always ask for 64-bit off_t. Using off_t instead of int64_t is probably a separate cleanup, but one that may be worth making prior to this patch, so I'll defer this one to my v3. > > Moreover: what is the reason for using off_t in NBD code? We don't have it > in NBD protocol, we don't have it in generic block layer interface. Isn't it > always casted to int64_t or like this? >
On 1/4/19 5:50 PM, Eric Blake wrote: >> hmm, as I understand, even if this compatibility exists, it's not a part >> of standard and nothing about off_t size in POSIX.. > > off_t allows you to run on older systems with 32-bit offsets and newer > systems with 64-bit offsets; but these days, even on 32-bit systems, we > compile qemu to always ask for 64-bit off_t. Using off_t instead of > int64_t is probably a separate cleanup, but one that may be worth making > prior to this patch, so I'll defer this one to my v3. > >> >> Moreover: what is the reason for using off_t in NBD code? We don't have it >> in NBD protocol, we don't have it in generic block layer interface. Isn't it >> always casted to int64_t or like this? Thanks again for asking this question. In auditing the use of off_t, I found that 'qemu-nbd -P 1' happily tries to read beyond the bounds of the BDS. Thankfully, I can't find an exploit (escaped the CVE bullet - no DoS assertion, overlarge malloc, information leak, or other nasty problem), merely a permanent EIO down the road once the client tries to access advertised available bytes; but I'm also adding a patch to my v3 series that does sanity checking (as we should NEVER blindly trust values in a potentially-malicious image as being in bounds, so that clients can't even connect to such images in the first place). [I also think that qemu-nbd -P is seldom-used...]
diff --git a/qemu-nbd.c b/qemu-nbd.c index 2807e132396..e3f739671b5 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -544,9 +544,8 @@ int main(int argc, char **argv) }; int ch; int opt_ind = 0; - char *end; int flags = BDRV_O_RDWR; - int partition = -1; + int partition = 0; int ret = 0; bool seen_cache = false; bool seen_discard = false; @@ -657,13 +656,9 @@ int main(int argc, char **argv) port = optarg; break; case 'o': - dev_offset = strtoll (optarg, &end, 0); - if (*end) { - error_report("Invalid offset `%s'", optarg); - exit(EXIT_FAILURE); - } - if (dev_offset < 0) { - error_report("Offset must be positive `%s'", optarg); + if (qemu_strtoi64(optarg, NULL, 0, &dev_offset) < 0 || + dev_offset < 0) { + error_report("Invalid offset '%s'", optarg); exit(EXIT_FAILURE); } break; @@ -685,13 +680,9 @@ int main(int argc, char **argv) flags &= ~BDRV_O_RDWR; break; case 'P': - partition = strtol(optarg, &end, 0); - if (*end) { - error_report("Invalid partition `%s'", optarg); - exit(EXIT_FAILURE); - } - if (partition < 1 || partition > 8) { - error_report("Invalid partition %d", partition); + if (qemu_strtoi(optarg, NULL, 0, &partition) < 0 || + partition < 1 || partition > 8) { + error_report("Invalid partition '%s'", optarg); exit(EXIT_FAILURE); } break; @@ -709,15 +700,11 @@ int main(int argc, char **argv) device = optarg; break; case 'e': - shared = strtol(optarg, &end, 0); - if (*end) { + if (qemu_strtoi(optarg, NULL, 0, &shared) < 0 || + shared < 1) { error_report("Invalid shared device number '%s'", optarg); exit(EXIT_FAILURE); } - if (shared < 1) { - error_report("Shared device number must be greater than 0"); - exit(EXIT_FAILURE); - } break; case 'f': fmt = optarg; @@ -1006,7 +993,7 @@ int main(int argc, char **argv) } fd_size -= dev_offset; - if (partition != -1) { + if (partition) { ret = find_partition(blk, partition, &dev_offset, &fd_size); if (ret < 0) { error_report("Could not find partition %d: %s", partition,
Our copy-and-pasted open-coding of strtol handling forgot to handle overflow conditions. Use qemu_strto*() instead. In the case of --partition, since we insist on a user-supplied partition to be non-zero, we can use 0 rather than -1 for our initial value to distinguish when a partition is not being served, for slightly more optimal code. The error messages for out-of-bounds values are less specific, but should not be a terrible loss in quality. Signed-off-by: Eric Blake <eblake@redhat.com> --- v2: Retitle, catch more uses of strtol [Hmm - this depends on int64_t and off_t being compatible; if they aren't that way on all platforms, I'll need a temporary variable] --- qemu-nbd.c | 33 ++++++++++----------------------- 1 file changed, 10 insertions(+), 23 deletions(-)