Re: [Qemu-devel] [PATCH v2 12/22] nbd/client: Improve error handling in nbd_negotiate_simple_meta_context()

[Eric Blake]

On 12/15/18 9:19 AM, Richard W.M. Jones wrote:
> On Sat, Dec 15, 2018 at 07:53:14AM -0600, Eric Blake wrote:
> > Always allocate space for the reply returned by the server and
> > hoist the trace earlier, as it is more interesting to trace the
> > server's reply (even if it is unexpected) than parroting our
> > request only on success.  After all, skipping the allocation
> > for a wrong size was merely a micro-optimization that only
> > benefitted a broken server, rather than the common case of a
> > compliant server that meets our expectations.
> >
> > Then turn the reply handling into a loop (even though we still
> > never iterate more than once), to make this code easier to use
> > when later patches do support multiple server replies.  This
> > changes the error message for a server with two replies (a
> > corner case we are unlikely to hit in practice) from:
> >
> > Unexpected reply type 4 (meta context), expected 0 (ack)
> >
> > to:
> >
> > Server replied with more than one context
> >
> > Signed-off-by: Eric Blake <address@hidden>
> >
> > ---
> > v2: split patch into easier-to-review pieces [Rich, Vladimir]
> > ---
> >   nbd/client.c | 16 ++++++++++++----
> >   1 file changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/nbd/client.c b/nbd/client.c
> > index bcccd5f555e..b6a85fc3ef8 100644
> > --- a/nbd/client.c
> > +++ b/nbd/client.c
> > @@ -684,10 +684,11 @@ static int nbd_negotiate_simple_meta_context(QIOChannel 
> > *ioc,
> >           return ret;
> >       }
> >
> > -    if (reply.type == NBD_REP_META_CONTEXT) {
> > +    while (reply.type == NBD_REP_META_CONTEXT) {
>
> I'm not sure I understand why this change is safe.
>
> As far as I can see reply.type is only updated in the loop by
> nbd_receive_option_reply, and that reads from the server, and so the
> server might keep sending NBD_REP_META_CONTEXT packets (instead of the
> expected NBD_REP_ACK), so it could now loop forever against a
> malicious server?  (This is not taking into account any later patches)

The loop can execute at most twice:

> Rich.
>
> >           char *name;
> >
> > -        if (reply.length != sizeof(info->context_id) + context_len) {
> > +        if (reply.length <= sizeof(info->context_id) ||
> > +            reply.length > NBD_MAX_BUFFER_SIZE) {
> >               error_setg(errp, "Failed to negotiate meta context '%s', server "
> >                          "answered with unexpected length %" PRIu32, context,
> >                          reply.length);
> > @@ -708,6 +709,15 @@ static int nbd_negotiate_simple_meta_context(QIOChannel 
> > *ioc,
> >               return -1;
> >           }
> >           name[reply.length] = '\0';
> > +        trace_nbd_opt_meta_reply(name, info->context_id);
> > +
> > +        if (received) {
> > +            error_setg(errp, "Server replied with more than one context");
> > +            g_free(name);
> > +            nbd_send_opt_abort(ioc);
> > +            return -1;
> > +        }

If the server replies with a second context, we break the loop by 
complaining.

The old code accepted at most one context, by complaining if the 
server's second reply was not ACK; the new code accepts at most one 
context, by complaining if the server sent more than one context, so the 
net effect of killing the connection for a misbehaving server response 
to SET is unchanged.

However, your point about a misbehaving server providing an infinite 
stream of responses to NBD_OPT_LIST or NBD_OPT_LIST_META_CONTEXT is an 
interesting question, and may be worth asking upstream to see if the NBD 
protocol should be tweaked to document any boundaries at how many 
listings a server might send before a client should worry about the 
server being malicious.  (Does not affect this patch, but pre-existing 
when we call nbd_receive_list() for servers that lack NBD_OPT_GO, and 
does impact the later patches in this series that call 
NBD_OPT_LIST_META_CONTEXT for 'qemu-nbd --list').

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