[dm-devel] [PATCH] multipathd: Fixed multipathd parameter invoking

[lixiaokeng]

Before we apply f812466f ("multipathd: more robust command parsing"),
multipathd help shows:
...
 map|multipath $map getprstatus
 map|multipath $map setprstatus
 map|multipath $map unsetprstatus
 map|multipath $map getprkey
 map|multipath $map setprkey key $key
 map|multipath $map unsetprkey
 forcequeueing daemon
 restorequeueing daemon
 path $path setmarginal
 path $path unsetmarginal
 map|multipath $map unsetmarginal

We think it is better to keep this consistent, so we fix it
as this patch. Besides that, we also change "getprstatus'
"getprkey" "setmarginal" "unsetmarginal" orders to keep
consistent and avoid same problem if users use "multipathd
xxx" (like getprkey).

On 2022/12/1 0:04, Benjamin Marzinski wrote:
> On Wed, Nov 30, 2022 at 05:13:28PM +0800, miaoguanqin wrote:
>> ping
> 
> I fixed this issue a different way. Previously we accepted any ordering
> of keywords, but we have always advertised a specific order in the CLI
> commands reference (to see it, just run "mulitpathd help" or use any
> other invalid keyword). The command word is first and the arguments
> follow.  I think we should keep this consistent, and I don't think we
> should go changing the order of existing commands. Instead, my patch
> makes libmpathpersist issue commands in the correct order. Could you
> look at:
> 
> [dm-devel] [PATCH 2/2] libmpathpersist: fix command keyword ordering
> https://listman.redhat.com/archives/dm-devel/2022-November/052773.html
> 
> and see if that solves your issue.
> 
> -Ben
> 
>>
>> On 2022/11/25 9:26, miaoguanqin wrote:
>>> Users may fail to execute command: multipathd and mpathpersist.
>>>
>>> When we execute the command mpathpersist：
>>> mpathpersist --out --register --param-sark=123 --prout-type=5
>>> /dev/mapper/mpathb
>>> It return an error : Missing arguement. The preceding command calls the
>>> function
>>> cli_setprkey, which is called by checking whether the handle values are
>>> consistent
>>> with the command input. CVE-2022-41974 changed the handler value of
>>> function and
>>> changed the mode of calculating handle.  The handler value is not equal to
>>> the
>>> command input, causing multipathd can not execute the true funcion. It
>>> could be
>>> an same error for executing multipathd by the old mode.
>>>
>>> multipathd invokes the corresponding function based on the handle value.
>>> CVE-2022-41964 changed the method of calculating handler value. Modify the
>>> handle
>>> value so that the corresponding function can be correctly execute.
>>>
>>> Signed-off-by: miaoguanqin <miaoguanqin at huawei.com>
>>> Signed-off-by: lixiaokeng <lixiaokeng at huawei.com>
>>> ---
>>>  multipathd/callbacks.c | 18 +++++++++---------
>>>  multipathd/cli.h       |  9 ++++++++-
>>>  2 files changed, 17 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/multipathd/callbacks.c b/multipathd/callbacks.c
>>> index fb87b280..f32666be 100644
>>> --- a/multipathd/callbacks.c
>>> +++ b/multipathd/callbacks.c
>>> @@ -57,16 +57,16 @@ void init_handler_callbacks(void)
>>>      set_handler_callback(VRB_RESTOREQ | Q1_MAPS,
>>> HANDLER(cli_restore_all_queueing));
>>>      set_unlocked_handler_callback(VRB_QUIT, HANDLER(cli_quit));
>>>      set_unlocked_handler_callback(VRB_SHUTDOWN, HANDLER(cli_shutdown));
>>> -    set_handler_callback(VRB_GETPRSTATUS | Q1_MAP,
>>> HANDLER(cli_getprstatus));
>>> -    set_handler_callback(VRB_SETPRSTATUS | Q1_MAP,
>>> HANDLER(cli_setprstatus));
>>> -    set_handler_callback(VRB_UNSETPRSTATUS | Q1_MAP,
>>> HANDLER(cli_unsetprstatus));
>>> +    set_handler_callback(KEY_MAP | Q1_GETPRSTATUS,
>>> HANDLER(cli_getprstatus));
>>> +    set_handler_callback(KEY_MAP | Q1_SETSTATUS,
>>> HANDLER(cli_setprstatus));
>>> +    set_handler_callback(KEY_MAP | Q1_UNSETSTATUS,
>>> HANDLER(cli_unsetprstatus));
>>>      set_handler_callback(VRB_FORCEQ | Q1_DAEMON,
>>> HANDLER(cli_force_no_daemon_q));
>>>      set_handler_callback(VRB_RESTOREQ | Q1_DAEMON,
>>> HANDLER(cli_restore_no_daemon_q));
>>> -    set_handler_callback(VRB_GETPRKEY | Q1_MAP, HANDLER(cli_getprkey));
>>> -    set_handler_callback(VRB_SETPRKEY | Q1_MAP | Q2_KEY,
>>> HANDLER(cli_setprkey));
>>> -    set_handler_callback(VRB_UNSETPRKEY | Q1_MAP,
>>> HANDLER(cli_unsetprkey));
>>> -    set_handler_callback(VRB_SETMARGINAL | Q1_PATH,
>>> HANDLER(cli_set_marginal));
>>> -    set_handler_callback(VRB_UNSETMARGINAL | Q1_PATH,
>>> HANDLER(cli_unset_marginal));
>>> -    set_handler_callback(VRB_UNSETMARGINAL | Q1_MAP,
>>> +    set_handler_callback(KEY_MAP | Q1_GETPRKEY, HANDLER(cli_getprkey));
>>> +    set_handler_callback(KEY_MAP | Q1_SETKEY | Q2_KEY,
>>> HANDLER(cli_setprkey));
>>> +    set_handler_callback(KEY_MAP | Q1_UNSETKEY, HANDLER(cli_unsetprkey));
>>> +    set_handler_callback(KEY_PATH | Q1_SETMARGINAL,
>>> HANDLER(cli_set_marginal));
>>> +    set_handler_callback(KEY_PATH | Q1_UNSETMARGINAL,
>>> HANDLER(cli_unset_marginal));
>>> +    set_handler_callback(KEY_MAP | Q1_UNSETMARGINAL,
>>>                   HANDLER(cli_unset_all_marginal));
>>>  }
>>> diff --git a/multipathd/cli.h b/multipathd/cli.h
>>> index c6b79c9d..08ee5c8d 100644
>>> --- a/multipathd/cli.h
>>> +++ b/multipathd/cli.h
>>> @@ -80,7 +80,14 @@ enum {
>>>      Q1_ALL            = KEY_ALL << 8,
>>>      Q1_DAEMON        = KEY_DAEMON << 8,
>>>      Q1_STATUS        = KEY_STATUS << 8,
>>> -
>>> +    Q1_SETKEY        = VRB_SETPRKEY << 8,
>>> +    Q1_UNSETKEY        = VRB_UNSETPRKEY << 8,
>>> +    Q1_SETSTATUS        = VRB_SETPRSTATUS << 8,
>>> +    Q1_UNSETSTATUS        = VRB_UNSETPRSTATUS << 8,
>>> +    Q1_GETPRSTATUS        = VRB_GETPRSTATUS << 8,
>>> +    Q1_GETPRKEY        = VRB_GETPRKEY << 8,
>>> +    Q1_SETMARGINAL        = VRB_SETMARGINAL << 8,
>>> +    Q1_UNSETMARGINAL    = VRB_UNSETMARGINAL << 8,
>>>      /* byte 2: qualifier 2 */
>>>      Q2_FMT            = KEY_FMT << 16,
>>>      Q2_RAW            = KEY_RAW << 16,
> 
> .
>