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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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@xxxxxxxxxx>
> CC: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
> CC: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> CC: Chao Peng <chao.p.peng@xxxxxxxxxxxxxxx>
> 

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

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel