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

[Ian Campbell <ian.campbell@citrix.com>]

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);
>