[Qemu-devel] [PATCH] net: avoid to use variable length array in net_client_init()

[Qemu-devel] [PATCH] net: avoid to use variable length array in net_client_init()

[Stefano Garzarella]

net_client_init() uses a variable length array to store the prefix
of 'ipv6-net' parameter (e.g. if ipv6-net=fec0::0/64, the prefix
is 'fec0::0').
Since the IPv6 prefix can be at most as long as an IPv6 address,
we can use an array with fixed size equals to INET6_ADDRSTRLEN.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 net/net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/net.c b/net/net.c
index f3a3c5444c..2e5f27e121 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1118,7 +1118,7 @@ static int net_client_init(QemuOpts *opts, bool is_netdev, Error **errp)
         const char *ip6_net = qemu_opt_get(opts, "ipv6-net");
 
         if (ip6_net) {
-            char buf[strlen(ip6_net) + 1];
+            char buf[INET6_ADDRSTRLEN];
 
             if (get_str_sep(buf, sizeof(buf), &ip6_net, '/') < 0) {
                 /* Default 64bit prefix length.  */
-- 
2.20.1

[Qemu-devel] [PATCH] net: avoid to use variable length array in net_client_init()

[Stefano Garzarella]

net_client_init() uses a variable length array to store the prefix
of 'ipv6-net' parameter (e.g. if ipv6-net=fec0::0/64, the prefix
is 'fec0::0').
Since the IPv6 prefix can be at most as long as an IPv6 address,
we can use an array with fixed size equals to INET6_ADDRSTRLEN.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 net/net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/net.c b/net/net.c
index f3a3c5444c..2e5f27e121 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1118,7 +1118,7 @@ static int net_client_init(QemuOpts *opts, bool is_netdev, Error **errp)
         const char *ip6_net = qemu_opt_get(opts, "ipv6-net");
 
         if (ip6_net) {
-            char buf[strlen(ip6_net) + 1];
+            char buf[INET6_ADDRSTRLEN];
 
             if (get_str_sep(buf, sizeof(buf), &ip6_net, '/') < 0) {
                 /* Default 64bit prefix length.  */
-- 
2.20.1

Re: [Qemu-devel] [PATCH] net: avoid to use variable length array in net_client_init()

[Markus Armbruster]

Stefano Garzarella <sgarzare@redhat.com> writes:

> net_client_init() uses a variable length array to store the prefix
> of 'ipv6-net' parameter (e.g. if ipv6-net=fec0::0/64, the prefix
> is 'fec0::0').
> Since the IPv6 prefix can be at most as long as an IPv6 address,
> we can use an array with fixed size equals to INET6_ADDRSTRLEN.
>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  net/net.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/net.c b/net/net.c
> index f3a3c5444c..2e5f27e121 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1118,7 +1118,7 @@ static int net_client_init(QemuOpts *opts, bool is_netdev, Error **errp)
>          const char *ip6_net = qemu_opt_get(opts, "ipv6-net");
>  
>          if (ip6_net) {
> -            char buf[strlen(ip6_net) + 1];
> +            char buf[INET6_ADDRSTRLEN];
>  
>              if (get_str_sep(buf, sizeof(buf), &ip6_net, '/') < 0) {
>                  /* Default 64bit prefix length.  */

Hmm.

Parameter "ipv6-net" is of the form ADDRESS[/PREFIX-SIZE].  If
/PREFIX-SIZE is present, get_str_sep() copies the ADDRESS part to buf[].

However, nothing stops the user from passing in an ADDRESS longer than
INET6_ADDRSTRLEN, say by adding a enough leading zeros.  get_str_sep()
will then silently truncate ADDRESS.

Suggest to avoid get_str_sep() like this (not even compile-tested):

        if (ip6_net) {
            char *slashp = strchr(ip6_net, '/');

            if (!slashp) {
                /* Default 64bit prefix length.  */
                qemu_opt_set(opts, "ipv6-prefix", ip6_net, &error_abort);
                qemu_opt_set_number(opts, "ipv6-prefixlen", 64, &error_abort);
            } else {
                /* User-specified prefix length.  */
                unsigned long len;
                int err;
                char *addr = g_strndup(ip6_net, slashp - ip6_net);

                qemu_opt_set(opts, "ipv6-prefix", addr, &error_abort);
                g_free(addr);
                err = qemu_strtoul(slashp + 1, NULL, 10, &len);
                if (err) {
                    error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                              "ipv6-prefix", "a number");
                } else {
                    qemu_opt_set_number(opts, "ipv6-prefixlen", len,
                                        &error_abort);
                }
            }
            qemu_opt_unset(opts, "ipv6-net");
        }
    }

I'd be tempted to clean up further; de-duplicate the qemu_opt_set() and
qemu_opt_set_number().

There's just one more use of get_str_sep(), in parse_host_port(), and it
looks just as prone to silent truncation.

Re: [Qemu-devel] [PATCH] net: avoid to use variable length array in net_client_init()

[Stefano Garzarella]

On Mon, May 06, 2019 at 03:23:08PM +0200, Markus Armbruster wrote:
> Stefano Garzarella <sgarzare@redhat.com> writes:
> 
> > net_client_init() uses a variable length array to store the prefix
> > of 'ipv6-net' parameter (e.g. if ipv6-net=fec0::0/64, the prefix
> > is 'fec0::0').
> > Since the IPv6 prefix can be at most as long as an IPv6 address,
> > we can use an array with fixed size equals to INET6_ADDRSTRLEN.
> >
> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > ---
> >  net/net.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/net.c b/net/net.c
> > index f3a3c5444c..2e5f27e121 100644
> > --- a/net/net.c
> > +++ b/net/net.c
> > @@ -1118,7 +1118,7 @@ static int net_client_init(QemuOpts *opts, bool is_netdev, Error **errp)
> >          const char *ip6_net = qemu_opt_get(opts, "ipv6-net");
> >  
> >          if (ip6_net) {
> > -            char buf[strlen(ip6_net) + 1];
> > +            char buf[INET6_ADDRSTRLEN];
> >  
> >              if (get_str_sep(buf, sizeof(buf), &ip6_net, '/') < 0) {
> >                  /* Default 64bit prefix length.  */
> 
> Hmm.
> 
> Parameter "ipv6-net" is of the form ADDRESS[/PREFIX-SIZE].  If
> /PREFIX-SIZE is present, get_str_sep() copies the ADDRESS part to buf[].
> 
> However, nothing stops the user from passing in an ADDRESS longer than
> INET6_ADDRSTRLEN, say by adding a enough leading zeros.  get_str_sep()
> will then silently truncate ADDRESS.
> 
> Suggest to avoid get_str_sep() like this (not even compile-tested):
> 
>         if (ip6_net) {
>             char *slashp = strchr(ip6_net, '/');
> 
>             if (!slashp) {
>                 /* Default 64bit prefix length.  */
>                 qemu_opt_set(opts, "ipv6-prefix", ip6_net, &error_abort);
>                 qemu_opt_set_number(opts, "ipv6-prefixlen", 64, &error_abort);
>             } else {
>                 /* User-specified prefix length.  */
>                 unsigned long len;
>                 int err;
>                 char *addr = g_strndup(ip6_net, slashp - ip6_net);
> 
>                 qemu_opt_set(opts, "ipv6-prefix", addr, &error_abort);
>                 g_free(addr);
>                 err = qemu_strtoul(slashp + 1, NULL, 10, &len);
>                 if (err) {
>                     error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>                               "ipv6-prefix", "a number");
>                 } else {
>                     qemu_opt_set_number(opts, "ipv6-prefixlen", len,
>                                         &error_abort);
>                 }
>             }
>             qemu_opt_unset(opts, "ipv6-net");
>         }
>     }

Thank you for the suggestion! It seems much better to me.
I'll follow your idea for the v2.

> 
> I'd be tempted to clean up further; de-duplicate the qemu_opt_set() and
> qemu_opt_set_number().

Yes, it seems simple to de-duplicate in this way:
        if (ip6_net) {
            char *slashp = strchr(ip6_net, '/');
            char *addr = ip6_net;
            unsigned long len = 64;

            if (slashp) {
                ...
            }

            qemu_opt_set(opts, "ipv6-prefix", addr, &error_abort);
            qemu_opt_set_number(opts, "ipv6-prefixlen", len, &error_abort);
            qemu_opt_unset(opts, "ipv6-net");
        }

> 
> There's just one more use of get_str_sep(), in parse_host_port(), and it
> looks just as prone to silent truncation.

I'll check it.

Thanks,
Stefano

Re: [Qemu-devel] [PATCH] net: avoid to use variable length array in net_client_init()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 558 bytes --]

On 5/3/19 12:06 PM, Stefano Garzarella wrote:
> net_client_init() uses a variable length array to store the prefix
> of 'ipv6-net' parameter (e.g. if ipv6-net=fec0::0/64, the prefix
> is 'fec0::0').
> Since the IPv6 prefix can be at most as long as an IPv6 address,
> we can use an array with fixed size equals to INET6_ADDRSTRLEN.

In addition to Markus' comments, on the subject line:

s/avoid to use/avoid using/

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH] net: avoid to use variable length array in net_client_init()

[Stefano Garzarella]

On Mon, May 06, 2019 at 12:54:20PM -0500, Eric Blake wrote:
> On 5/3/19 12:06 PM, Stefano Garzarella wrote:
> > net_client_init() uses a variable length array to store the prefix
> > of 'ipv6-net' parameter (e.g. if ipv6-net=fec0::0/64, the prefix
> > is 'fec0::0').
> > Since the IPv6 prefix can be at most as long as an IPv6 address,
> > we can use an array with fixed size equals to INET6_ADDRSTRLEN.
> 
> In addition to Markus' comments, on the subject line:
> 
> s/avoid to use/avoid using/
> 

Thanks, I'll change it!

Stefano