Re: [Qemu-devel] [PATCH 2/5] nbd/server: add nbd_opt_{read, drop} to track client->optlen

[Eric Blake]

On 11/22/2017 02:03 PM, Eric Blake wrote:

On 11/22/2017 04:19 AM, Vladimir Sementsov-Ogievskiy wrote:

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  nbd/server.c | 34 ++
  1 file changed, 22 insertions(+), 12 deletions(-)






I think that means we need a tri-state return from nbd_opt_read(): < 0
on I/O failure, 0 if the EINVAL message has been sent, and 1 if the read
was successful; I also think it means that the handler for
NBD_OPT_EXPORT_NAME does not really fit the bill for using the same
handler.  Hopefully this explanation will give you more insight into the
counter-proposal patch I'm about to post.


Hmm, I never posted the counter-proposal, because I got caught up in 
handling the CVEs that I spotted in the same area, then the 2.11 
release.  I'll get back to this series shortly.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 2/5] nbd/server: add nbd_opt_{read, drop} to track client->optlen

[Eric Blake]

On 11/22/2017 04:19 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  nbd/server.c | 34 ++
>  1 file changed, 22 insertions(+), 12 deletions(-)
> 

Hmm, revisiting my idea about moving more of the error checking into the
helper function.  As of this patch, we only have two clients of
nbd_opt_read:

> @@ -299,7 +312,7 @@ static int nbd_negotiate_handle_export_name(NBDClient 
> *client,
>  error_setg(errp, "Bad length received");
>  return -EINVAL;
>  }
> -if (nbd_read(client->ioc, name, client->optlen, errp) < 0) {
> +if (nbd_opt_read(client, name, client->optlen, errp) < 0) {
>  error_prepend(errp, "read failed: ");
>  return -EINVAL;

1. NBD_OPT_EXPORT_NAME, where the length check is based on the maximum
size name we're willing to accept (and NOT on comparison to the header
size, as we request the entire client->optlen).  This call cannot send
an error reply (so if the length is bogus, we can just drop the
connection by returning -EINVAL).  Furthermore, once we handle this
option, option processing is done; we do not reach the assert that
client->optlen == 0.

>  }
> @@ -383,40 +396,36 @@ static int nbd_negotiate_handle_info(NBDClient *client, 
> uint16_t myflags,
>  msg = "overall request too short";
>  goto invalid;
>  }
> -if (nbd_read(client->ioc, , sizeof(namelen), errp) < 0) {
> +if (nbd_opt_read(client, , sizeof(namelen), errp) < 0) {
>  return -EIO;
>  }

2. Multiple calls within NBD_OPT_INFO/NBD_OPT_GO.  Here, the length
check is based on our read being a subset of client->optlen, and our
desired response on a failed check is whatever is at the invalid: label,
namely:

 invalid:
if (nbd_opt_drop(client, client->optlen, errp) < 0) {
return -EIO;
}
return nbd_negotiate_send_rep_err(client, NBD_REP_ERR_INVALID,
  errp, "%s", msg);

We want to drop all remaining data, reply to the client, and return 0 to
keep the connection alive (or -EIO if the read or write failed).

You are planning on adding more calls withing NBD_OPT_LIST_META_CONTEXT,
which will have semantics more like 2 (if we detect an inconsistent
size, we want to consume the rest of the input and return an EINVAL
reply to the client, but keep the connection alive unless there is an
I/O error in the meantime).

I think that means we need a tri-state return from nbd_opt_read(): < 0
on I/O failure, 0 if the EINVAL message has been sent, and 1 if the read
was successful; I also think it means that the handler for
NBD_OPT_EXPORT_NAME does not really fit the bill for using the same
handler.  Hopefully this explanation will give you more insight into the
counter-proposal patch I'm about to post.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 2/5] nbd/server: add nbd_opt_{read, drop} to track client->optlen

[Eric Blake]

On 11/22/2017 04:19 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  nbd/server.c | 34 ++
>  1 file changed, 22 insertions(+), 12 deletions(-)
> @@ -299,7 +312,7 @@ static int nbd_negotiate_handle_export_name(NBDClient 
> *client,
>  error_setg(errp, "Bad length received");
>  return -EINVAL;
>  }
> -if (nbd_read(client->ioc, name, client->optlen, errp) < 0) {
> +if (nbd_opt_read(client, name, client->optlen, errp) < 0) {
>  error_prepend(errp, "read failed: ");
>  return -EINVAL;
>  }

More context:

  name[client->optlen] = '\0';

Oops - that's broken, because client->optlen is now 0.  Which means your
code was only tested with empty-string default exports.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 2/5] nbd/server: add nbd_opt_{read, drop} to track client->optlen

[Eric Blake]

On 11/22/2017 04:19 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy 

The subject line says what and sort of implies why, but the commit
message body is rather sparse.

> ---
>  nbd/server.c | 34 ++
>  1 file changed, 22 insertions(+), 12 deletions(-)

Not a net win in lines of code, so a bit more justification may be helpful.

> 
> diff --git a/nbd/server.c b/nbd/server.c
> index bccc0274e7..c9445a89e9 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -139,6 +139,19 @@ static void nbd_client_receive_next_request(NBDClient 
> *client);
>  
>  */
>  
> +static inline int nbd_opt_read(NBDClient *client, void *buffer, size_t size,
> +   Error **errp)
> +{
> +client->optlen -= size;
> +return qio_channel_read_all(client->ioc, buffer, size, errp) < 0 ? -EIO 
> : 0;

Should this code check that client->optlen >= size, and issue the
appropriate error message if not? That may centralize some of the error
checking repeated elsewhere.

> +}
> +
> +static inline int nbd_opt_drop(NBDClient *client, size_t size, Error **errp)
> +{
> +client->optlen -= size;
> +return nbd_drop(client->ioc, size, errp);

Same question on size validation.

> +}
> +
>  /* Send a reply header, including length, but no payload.
>   * Return -errno on error, 0 on success. */
>  static int nbd_negotiate_send_rep_len(NBDClient *client, uint32_t type,
> @@ -299,7 +312,7 @@ static int nbd_negotiate_handle_export_name(NBDClient 
> *client,
>  error_setg(errp, "Bad length received");
>  return -EINVAL;
>  }
> -if (nbd_read(client->ioc, name, client->optlen, errp) < 0) {
> +if (nbd_opt_read(client, name, client->optlen, errp) < 0) {
>  error_prepend(errp, "read failed: ");
>  return -EINVAL;

Here's an example caller that had a previous size validation.

>  }
> @@ -383,40 +396,36 @@ static int nbd_negotiate_handle_info(NBDClient *client, 
> uint16_t myflags,
>  msg = "overall request too short";
>  goto invalid;
>  }
> -if (nbd_read(client->ioc, , sizeof(namelen), errp) < 0) {
> +if (nbd_opt_read(client, , sizeof(namelen), errp) < 0) {
>  return -EIO;

And again, a size validation right before the call.

>  }
>  be32_to_cpus();
> -client->optlen -= sizeof(namelen);

Okay, part of the refactoring means you don't have to remember to track
remaining size separately from reading; that's a nice feature.  I
suspect it is also possible to assert that client->optlen is zero before
reading the next opt (I'm reviewing the patch in order, we'll see if I
come back here). [1]

>  if (namelen > client->optlen - sizeof(requests) ||
>  (client->optlen - namelen) % 2)
>  {
>  msg = "name length is incorrect";
>  goto invalid;
>  }
> -if (nbd_read(client->ioc, name, namelen, errp) < 0) {
> +if (nbd_opt_read(client, name, namelen, errp) < 0) {
>  return -EIO;
>  }

Another size check prior to the call (actually checking multiple
conditions)...

>  name[namelen] = '\0';
> -client->optlen -= namelen;
>  trace_nbd_negotiate_handle_export_name_request(name);
>  
> -if (nbd_read(client->ioc, , sizeof(requests), errp) < 0) {
> +if (nbd_opt_read(client, , sizeof(requests), errp) < 0) {
>  return -EIO;

...and no direct size check before this call, because the earlier size
checks already covered it.  Arguably, the in-place size check error
messages may be slightly more precise, but any message at all about
unexpected sizing is appropriate (given that only broken clients should
be sending incorrect sizes) - so I'm still leaning towards a stronger
refactoring that puts the size check in the helper function.

> @@ -521,7 +530,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, 
> uint16_t myflags,
>  return rc;
>  
>   invalid:
> -if (nbd_drop(client->ioc, client->optlen, errp) < 0) {
> +if (nbd_opt_drop(client, client->optlen, errp) < 0) {
>  return -EIO;
>  }
>  return nbd_negotiate_send_rep_err(client, NBD_REP_ERR_INVALID,
> @@ -715,7 +724,7 @@ static int nbd_negotiate_options(NBDClient *client, 
> uint16_t myflags,
>  return -EINVAL;
>  
>  default:
> -if (nbd_drop(client->ioc, length, errp) < 0) {
> +if (nbd_opt_drop(client, length, errp) < 0) {

Isn't length == client->optlen here? If so, can we omit the length
parameter to nbd_opt_drop(), and instead have it defined as dropping to
the end of the current option?

> @@ -821,6 +830,7 @@ static int nbd_negotiate_options(NBDClient *client, 
> uint16_t myflags,
>  if (ret < 0) {
>  return ret;
>  }
> +assert(client->optlen == 0);

[1] Ah, you did add the assertion.

I'm going to propose a variant on this patch, to cover the points I made
above about ease-of-use (and to hopefully