Re: [PATCH] dm-pcache: reject option groups without values

[Zheng Gu]

Hi Samuel,

Thanks for catching this issue.

On Tue, Jun 9, 2026 at 8:31 AM Samuel Moelius
<sam.moelius@xxxxxxxxxxxxxxx> wrote:
>
> The pcache target parses optional arguments as name/value pairs.  A
> table that advertises one optional argument and supplies only a
> recognized option name, for example "cache_mode", reaches
> parse_cache_opts() with argc == 1.  The parser consumes the name,
> decrements argc to zero, then calls dm_shift_arg() again for the value.
> dm_shift_arg() returns NULL when no arguments remain, and the following
> strcmp() dereferences that NULL pointer.
>
> Reject odd optional argument counts before parsing the name/value pairs.
> This keeps valid "cache_mode writeback" and "data_crc true/false" tables
> unchanged while making malformed tables fail during target construction.
>
> Assisted-by: Codex:gpt-5.5-cyber-preview
> Signed-off-by: Samuel Moelius <sam.moelius@xxxxxxxxxxxxxxx>
> ---
>  drivers/md/dm-pcache/dm_pcache.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/md/dm-pcache/dm_pcache.c b/drivers/md/dm-pcache/dm_pcache.c
> index 81c795c0400e..ba5471885b16 100644
> --- a/drivers/md/dm-pcache/dm_pcache.c
> +++ b/drivers/md/dm-pcache/dm_pcache.c
> @@ -163,6 +163,11 @@ static int parse_cache_opts(struct dm_pcache *pcache, struct dm_arg_set *as,
>         if (ret)
>                 return -EINVAL;
>
> +       if (argc & 1) {
> +               *error = "Invalid number of cache option arguments";
> +               return -EINVAL;
> +       }
> +
The vulnerability regarding missing values for cache options is indeed
real and poses
a kernel panic risk due to potential NULL pointer dereference or
underflowing the loop counter.
However, resolving this using a rigid parity check (argc & 1) might
not be the most robust approach.
Instead of an aggressive top-level parity filter, it would be much
cleaner and more idiomatic to enforce
strict bounds validation and exact argument pairing inside the evaluation loop.
>         while (argc) {
>                 arg = dm_shift_arg(as);
>                 argc--;
+                       if (!argc) {
+                               *error = "Missing value for cache_mode";
+                               return -EINVAL;
+                       }
This way, we properly keep the argument parsing framework flexible for
future non-paired extensions.
What do you think?

Best regards,
Gu
> --
> 2.43.0
>