[PATCH] tools/xl: Fix segfaults from `xl psr-cat-cbm-set` command line handling

[PATCH] tools/xl: Fix segfaults from `xl psr-cat-cbm-set` command line handling

[Andrew Cooper]

The socket option takes a mandatory argument.  Mark it as such, so
optarg isn't NULL when passed to trim(), which unconditionally
dereference it.

Range check optind against argc before blindly assuming that
argv[optind] and argv[optind+1] exist.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Chao Peng <chao.p.peng@linux.intel.com>

---

I started doing an audit of xl's command line handling, but got to the
very first command (memmax) and found another segfault because of
blindly assuming that argv[optind + 1] was available.

I fixed this example as I happened to use the command, but I currently
lack the time to do a complete audit.  IMO, a full audit should be a
blocker for 4.6, especially given the nature of XSA-137
---
 tools/libxl/xl_cmdimpl.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 37d4af6..f778cbe 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -8395,7 +8395,7 @@ int main_psr_cat_cbm_set(int argc, char **argv)
     int i, j, len;
 
     static struct option opts[] = {
-        {"socket", 0, 0, 's'},
+        {"socket", required_argument, 0, 's'},
         COMMON_LONG_OPTS,
         {0, 0, 0, 0}
     };
@@ -8403,7 +8403,7 @@ int main_psr_cat_cbm_set(int argc, char **argv)
     libxl_socket_bitmap_alloc(ctx, &target_map, 0);
     libxl_bitmap_set_none(&target_map);
 
-    SWITCH_FOREACH_OPT(opt, "s", opts, "psr-cat-cbm-set", 1) {
+    SWITCH_FOREACH_OPT(opt, "s:", opts, "psr-cat-cbm-set", 1) {
     case 's':
         trim(isspace, optarg, &value);
         split_string_into_string_list(value, ",", &socket_list);
@@ -8422,6 +8422,11 @@ int main_psr_cat_cbm_set(int argc, char **argv)
     if (libxl_bitmap_is_empty(&target_map))
         libxl_bitmap_set_any(&target_map);
 
+    if (argc != optind + 2) {
+        help("psr-cat-cbm-set");
+        return 2;
+    }
+
     domid = find_domain(argv[optind]);
     cbm = strtoll(argv[optind + 1], NULL , 0);
 
-- 
1.7.10.4

Re: [PATCH] tools/xl: Fix segfaults from `xl psr-cat-cbm-set` command line handling

[Wei Liu]

On Thu, Jul 16, 2015 at 08:32:45PM +0100, Andrew Cooper wrote:
> The socket option takes a mandatory argument.  Mark it as such, so
> optarg isn't NULL when passed to trim(), which unconditionally
> dereference it.
> 
> Range check optind against argc before blindly assuming that
> argv[optind] and argv[optind+1] exist.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Chao Peng <chao.p.peng@linux.intel.com>
> 

Acked-by: Wei Liu <wei.liu2@citrix.com>

> ---
> 
> I started doing an audit of xl's command line handling, but got to the
> very first command (memmax) and found another segfault because of
> blindly assuming that argv[optind + 1] was available.
> 
> I fixed this example as I happened to use the command, but I currently
> lack the time to do a complete audit.  IMO, a full audit should be a
> blocker for 4.6, especially given the nature of XSA-137

I'll have a look at Coverity scan after things settle down a bit.

> ---
>  tools/libxl/xl_cmdimpl.c |    9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 37d4af6..f778cbe 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -8395,7 +8395,7 @@ int main_psr_cat_cbm_set(int argc, char **argv)
>      int i, j, len;
>  
>      static struct option opts[] = {
> -        {"socket", 0, 0, 's'},

Maybe I should send a patch to change first "0" to "no_argument" and
second "0" to NULL so that people take more care when copy-n-paste
things around.

> +        {"socket", required_argument, 0, 's'},
>          COMMON_LONG_OPTS,
>          {0, 0, 0, 0}
>      };
> @@ -8403,7 +8403,7 @@ int main_psr_cat_cbm_set(int argc, char **argv)
>      libxl_socket_bitmap_alloc(ctx, &target_map, 0);
>      libxl_bitmap_set_none(&target_map);
>  
> -    SWITCH_FOREACH_OPT(opt, "s", opts, "psr-cat-cbm-set", 1) {
> +    SWITCH_FOREACH_OPT(opt, "s:", opts, "psr-cat-cbm-set", 1) {
>      case 's':
>          trim(isspace, optarg, &value);
>          split_string_into_string_list(value, ",", &socket_list);
> @@ -8422,6 +8422,11 @@ int main_psr_cat_cbm_set(int argc, char **argv)
>      if (libxl_bitmap_is_empty(&target_map))
>          libxl_bitmap_set_any(&target_map);
>  
> +    if (argc != optind + 2) {
> +        help("psr-cat-cbm-set");
> +        return 2;
> +    }
> +
>      domid = find_domain(argv[optind]);
>      cbm = strtoll(argv[optind + 1], NULL , 0);
>  
> -- 
> 1.7.10.4

Re: [PATCH] tools/xl: Fix segfaults from `xl psr-cat-cbm-set` command line handling

[Ian Campbell]

On Thu, 2015-07-16 at 20:32 +0100, Andrew Cooper wrote:
> The socket option takes a mandatory argument.  Mark it as such, so
> optarg isn't NULL when passed to trim(), which unconditionally
> dereference it.
> 
> Range check optind against argc before blindly assuming that
> argv[optind] and argv[optind+1] exist.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Chao Peng <chao.p.peng@linux.intel.com>
> 
> ---
> 
> I started doing an audit of xl's command line handling, but got to the
> very first command (memmax) and found another segfault because of
> blindly assuming that argv[optind + 1] was available.
> 
> I fixed this example as I happened to use the command, but I currently
> lack the time to do a complete audit.  IMO, a full audit should be a
> blocker for 4.6, especially given the nature of XSA-137
> ---
>  tools/libxl/xl_cmdimpl.c |    9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 37d4af6..f778cbe 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -8395,7 +8395,7 @@ int main_psr_cat_cbm_set(int argc, char **argv)
>      int i, j, len;
>  
>      static struct option opts[] = {
> -        {"socket", 0, 0, 's'},
> +        {"socket", required_argument, 0, 's'},

I think we have failed to do this almost everywhere.

I think because people (like me) expected that giving : to the
corresponding short argument was sufficient, when that is not in fact
the case: val='s' here is an opaque cookie as far as the long opts are
concerned, not a reference of any kind to the short opts string, we just
happen to arrange for them to match for convenience.

In this case the short opt was wrong too, but anyway.

>          COMMON_LONG_OPTS,
>          {0, 0, 0, 0}
>      };
> @@ -8403,7 +8403,7 @@ int main_psr_cat_cbm_set(int argc, char **argv)
>      libxl_socket_bitmap_alloc(ctx, &target_map, 0);
>      libxl_bitmap_set_none(&target_map);
>  
> -    SWITCH_FOREACH_OPT(opt, "s", opts, "psr-cat-cbm-set", 1) {
> +    SWITCH_FOREACH_OPT(opt, "s:", opts, "psr-cat-cbm-set", 1) {
>      case 's':
>          trim(isspace, optarg, &value);
>          split_string_into_string_list(value, ",", &socket_list);
> @@ -8422,6 +8422,11 @@ int main_psr_cat_cbm_set(int argc, char **argv)
>      if (libxl_bitmap_is_empty(&target_map))
>          libxl_bitmap_set_any(&target_map);
>  
> +    if (argc != optind + 2) {

This implies that the final argument to SWITCH_FOREACH_OPT (the number
of required arguments after the options) is wrong and should be fixed
instead.

> +        help("psr-cat-cbm-set");
> +        return 2;
> +    }
> +
>      domid = find_domain(argv[optind]);
>      cbm = strtoll(argv[optind + 1], NULL , 0);
>  

Re: [PATCH] tools/xl: Fix segfaults from `xl psr-cat-cbm-set` command line handling

[Ian Jackson]

Andrew Cooper writes ("[PATCH] tools/xl: Fix segfaults from `xl psr-cat-cbm-set` command line handling"):
> The socket option takes a mandatory argument.  Mark it as such, so
> optarg isn't NULL when passed to trim(), which unconditionally
> dereference it.
> 
> Range check optind against argc before blindly assuming that
> argv[optind] and argv[optind+1] exist.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

> I started doing an audit of xl's command line handling, but got to the
> very first command (memmax) and found another segfault because of
> blindly assuming that argv[optind + 1] was available.
> 
> I fixed this example as I happened to use the command, but I currently
> lack the time to do a complete audit.  IMO, a full audit should be a
> blocker for 4.6, especially given the nature of XSA-137

Yes.  See also the 0/ message for my other fixes in this area.  (I
mention this since what I did was part of such an audit but probably
not a complete one.)

Ian.