RE: [PATCH 2/2] dm verity: allow only one verify mode

RE: [PATCH 2/2] dm verity: allow only one verify mode

[JeongHyeon Lee]

[-- Attachment #1: Type: text/plain, Size: 4442 bytes --]

Hello, Dear Sami Tolvanen.
Thank you for reply. Sorry, I send it again because my setting is wrong.

> I agree that we shouldn't allow this, at least not without a warning, but
> out of curiosity, do you actually have a situation where this could happen?
> One ideally shouldn't be passing untrusted parameters to dm-verity.

Of course, I don't think this will happen because they are dm-verity experts.
But since we are humans, I think this case could happen accidentally.
So it would be a good at preventing these cases.

> I don't have a strong opinion about this, but the documentation doesn't
> talk about verity modes, so perhaps this could be reworded to something
> like "Conflicting error handling parameters"?

Yes of course. That looks better.

I also had some ambiguous about how to express it. 
This is because I couldn't find it in document. 
The code says verity mode, so I wrote it down. never mind it :) 

like this)
    case DM_VERITY_MODE_LOGGING:
    case DM_VERITY_MODE_RESTART:
    case DM_VERITY_MODE_PANIC:

Thank you,
JeongHyeon Lee.

> On Thu, Mar 11, 2021 at 4:19 AM JeongHyeon Lee <jhs2.lee@samsung.com>
> wrote:
> >
> > If there are multiple verity mode when parsing the verity mode of dm
> > verity table, it will be set as the last one.
> > So set to 'allow only once' to prevent it.
> 
> I agree that we shouldn't allow this, at least not without a warning, but
> out of curiosity, do you actually have a situation where this could happen?
> One ideally shouldn't be passing untrusted parameters to dm-verity.
> 
> >
> > Signed-off-by: JeongHyeon Lee <jhs2.lee@samsung.com>
> > ---
> >  drivers/md/dm-verity-target.c | 38
> > ++++++++++++++++++++++++++---------
> >  1 file changed, 28 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/md/dm-verity-target.c
> > b/drivers/md/dm-verity-target.c index 808a98ef624c..b76431dc7721
> > 100644
> > --- a/drivers/md/dm-verity-target.c
> > +++ b/drivers/md/dm-verity-target.c
> > @@ -893,6 +893,28 @@ static int verity_alloc_zero_digest(struct
> dm_verity *v)
> >         return r;
> >  }
> >
> > +static inline bool verity_is_verity_mode(const char *arg_name) {
> > +       return (!strcasecmp(arg_name, DM_VERITY_OPT_LOGGING) ||
> > +               !strcasecmp(arg_name, DM_VERITY_OPT_RESTART) ||
> > +               !strcasecmp(arg_name, DM_VERITY_OPT_PANIC)); }
> > +
> > +static int verity_parse_verity_mode(struct dm_verity *v, const char
> > +*arg_name) {
> > +       if (v->mode)
> > +               return -EINVAL;
> > +
> > +       if (!strcasecmp(arg_name, DM_VERITY_OPT_LOGGING))
> > +               v->mode = DM_VERITY_MODE_LOGGING;
> > +       else if (!strcasecmp(arg_name, DM_VERITY_OPT_RESTART))
> > +               v->mode = DM_VERITY_MODE_RESTART;
> > +       else if (!strcasecmp(arg_name, DM_VERITY_OPT_PANIC))
> > +               v->mode = DM_VERITY_MODE_PANIC;
> > +
> > +       return 0;
> > +}
> > +
> >  static int verity_parse_opt_args(struct dm_arg_set *as, struct
> dm_verity *v,
> >                                  struct dm_verity_sig_opts
> > *verify_args)  { @@ -916,16 +938,12 @@ static int
> > verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
> >                 arg_name = dm_shift_arg(as);
> >                 argc--;
> >
> > -               if (!strcasecmp(arg_name, DM_VERITY_OPT_LOGGING)) {
> > -                       v->mode = DM_VERITY_MODE_LOGGING;
> > -                       continue;
> > -
> > -               } else if (!strcasecmp(arg_name, DM_VERITY_OPT_RESTART)) {
> > -                       v->mode = DM_VERITY_MODE_RESTART;
> > -                       continue;
> > -
> > -               } else if (!strcasecmp(arg_name, DM_VERITY_OPT_PANIC)) {
> > -                       v->mode = DM_VERITY_MODE_PANIC;
> > +               if (verity_is_verity_mode(arg_name)) {
> > +                       r = verity_parse_verity_mode(v, arg_name);
> > +                       if (r) {
> > +                               ti->error = "Already verity mode set";
> 
> I don't have a strong opinion about this, but the documentation doesn't
> talk about verity modes, so perhaps this could be reworded to something
> like "Conflicting error handling parameters"?
> 
> > +                               return r;
> > +                       }
> >                         continue;
> >
> >                 } else if (!strcasecmp(arg_name,
> > DM_VERITY_OPT_IGN_ZEROES)) {
> > --
> > 2.17.1
> >
> 
> Sami


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 1/2] dm verity: fix DM_VERITY_OPTS_MAX value

[JeongHyeon Lee]

Three optional parameters at once using dm verity table is not
supported, option can only be used exclusively.

e.g.)
  (verity_mode) (ignore_zero_block) (check_at_most_once)

verity table: '1  /dev/sdb /dev/sdb 4096 4096 1028807 1028807
<hash_alg> <root_hash> <salt> 11 use_fec_from_device /dev/sdb
fec_roots 2 fec_blocks 1036909 fec_start 1036909
restart_on_corruption ignore_zero_blocks check_at_most_once'

Signed-off-by: JeongHyeon Lee <jhs2.lee@samsung.com>
---
 drivers/md/dm-verity-target.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
index 6b8e5bdd8526..808a98ef624c 100644
--- a/drivers/md/dm-verity-target.c
+++ b/drivers/md/dm-verity-target.c
@@ -34,7 +34,7 @@
 #define DM_VERITY_OPT_IGN_ZEROES	"ignore_zero_blocks"
 #define DM_VERITY_OPT_AT_MOST_ONCE	"check_at_most_once"
 
-#define DM_VERITY_OPTS_MAX		(2 + DM_VERITY_OPTS_FEC + \
+#define DM_VERITY_OPTS_MAX		(3 + DM_VERITY_OPTS_FEC + \
 					 DM_VERITY_ROOT_HASH_VERIFICATION_OPTS)
 
 static unsigned dm_verity_prefetch_cluster = DM_VERITY_DEFAULT_PREFETCH_SIZE;
-- 
2.17.1

[PATCH 2/2] dm verity: allow only one verify mode

[JeongHyeon Lee]

If there are multiple verity mode when parsing the verity mode of dm
verity table, it will be set as the last one.
So set to 'allow only once' to prevent it.

Signed-off-by: JeongHyeon Lee <jhs2.lee@samsung.com>
---
 drivers/md/dm-verity-target.c | 38 ++++++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 10 deletions(-)

diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
index 808a98ef624c..b76431dc7721 100644
--- a/drivers/md/dm-verity-target.c
+++ b/drivers/md/dm-verity-target.c
@@ -893,6 +893,28 @@ static int verity_alloc_zero_digest(struct dm_verity *v)
 	return r;
 }
 
+static inline bool verity_is_verity_mode(const char *arg_name)
+{
+	return (!strcasecmp(arg_name, DM_VERITY_OPT_LOGGING) ||
+		!strcasecmp(arg_name, DM_VERITY_OPT_RESTART) ||
+		!strcasecmp(arg_name, DM_VERITY_OPT_PANIC));
+}
+
+static int verity_parse_verity_mode(struct dm_verity *v, const char *arg_name)
+{
+	if (v->mode)
+		return -EINVAL;
+
+	if (!strcasecmp(arg_name, DM_VERITY_OPT_LOGGING))
+		v->mode = DM_VERITY_MODE_LOGGING;
+	else if (!strcasecmp(arg_name, DM_VERITY_OPT_RESTART))
+		v->mode = DM_VERITY_MODE_RESTART;
+	else if (!strcasecmp(arg_name, DM_VERITY_OPT_PANIC))
+		v->mode = DM_VERITY_MODE_PANIC;
+
+	return 0;
+}
+
 static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
 				 struct dm_verity_sig_opts *verify_args)
 {
@@ -916,16 +938,12 @@ static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
 		arg_name = dm_shift_arg(as);
 		argc--;
 
-		if (!strcasecmp(arg_name, DM_VERITY_OPT_LOGGING)) {
-			v->mode = DM_VERITY_MODE_LOGGING;
-			continue;
-
-		} else if (!strcasecmp(arg_name, DM_VERITY_OPT_RESTART)) {
-			v->mode = DM_VERITY_MODE_RESTART;
-			continue;
-
-		} else if (!strcasecmp(arg_name, DM_VERITY_OPT_PANIC)) {
-			v->mode = DM_VERITY_MODE_PANIC;
+		if (verity_is_verity_mode(arg_name)) {
+			r = verity_parse_verity_mode(v, arg_name);
+			if (r) {
+				ti->error = "Already verity mode set";
+				return r;
+			}
 			continue;
 
 		} else if (!strcasecmp(arg_name, DM_VERITY_OPT_IGN_ZEROES)) {
-- 
2.17.1

Re: [PATCH 2/2] dm verity: allow only one verify mode

[Sami Tolvanen]

On Thu, Mar 11, 2021 at 4:19 AM JeongHyeon Lee <jhs2.lee@samsung.com> wrote:
>
> If there are multiple verity mode when parsing the verity mode of dm
> verity table, it will be set as the last one.
> So set to 'allow only once' to prevent it.

I agree that we shouldn't allow this, at least not without a warning,
but out of curiosity, do you actually have a situation where this
could happen? One ideally shouldn't be passing untrusted parameters to
dm-verity.

>
> Signed-off-by: JeongHyeon Lee <jhs2.lee@samsung.com>
> ---
>  drivers/md/dm-verity-target.c | 38 ++++++++++++++++++++++++++---------
>  1 file changed, 28 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
> index 808a98ef624c..b76431dc7721 100644
> --- a/drivers/md/dm-verity-target.c
> +++ b/drivers/md/dm-verity-target.c
> @@ -893,6 +893,28 @@ static int verity_alloc_zero_digest(struct dm_verity *v)
>         return r;
>  }
>
> +static inline bool verity_is_verity_mode(const char *arg_name)
> +{
> +       return (!strcasecmp(arg_name, DM_VERITY_OPT_LOGGING) ||
> +               !strcasecmp(arg_name, DM_VERITY_OPT_RESTART) ||
> +               !strcasecmp(arg_name, DM_VERITY_OPT_PANIC));
> +}
> +
> +static int verity_parse_verity_mode(struct dm_verity *v, const char *arg_name)
> +{
> +       if (v->mode)
> +               return -EINVAL;
> +
> +       if (!strcasecmp(arg_name, DM_VERITY_OPT_LOGGING))
> +               v->mode = DM_VERITY_MODE_LOGGING;
> +       else if (!strcasecmp(arg_name, DM_VERITY_OPT_RESTART))
> +               v->mode = DM_VERITY_MODE_RESTART;
> +       else if (!strcasecmp(arg_name, DM_VERITY_OPT_PANIC))
> +               v->mode = DM_VERITY_MODE_PANIC;
> +
> +       return 0;
> +}
> +
>  static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
>                                  struct dm_verity_sig_opts *verify_args)
>  {
> @@ -916,16 +938,12 @@ static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
>                 arg_name = dm_shift_arg(as);
>                 argc--;
>
> -               if (!strcasecmp(arg_name, DM_VERITY_OPT_LOGGING)) {
> -                       v->mode = DM_VERITY_MODE_LOGGING;
> -                       continue;
> -
> -               } else if (!strcasecmp(arg_name, DM_VERITY_OPT_RESTART)) {
> -                       v->mode = DM_VERITY_MODE_RESTART;
> -                       continue;
> -
> -               } else if (!strcasecmp(arg_name, DM_VERITY_OPT_PANIC)) {
> -                       v->mode = DM_VERITY_MODE_PANIC;
> +               if (verity_is_verity_mode(arg_name)) {
> +                       r = verity_parse_verity_mode(v, arg_name);
> +                       if (r) {
> +                               ti->error = "Already verity mode set";

I don't have a strong opinion about this, but the documentation
doesn't talk about verity modes, so perhaps this could be reworded to
something like "Conflicting error handling parameters"?

> +                               return r;
> +                       }
>                         continue;
>
>                 } else if (!strcasecmp(arg_name, DM_VERITY_OPT_IGN_ZEROES)) {
> --
> 2.17.1
>

Sami

RE: [PATCH 2/2] dm verity: allow only one verify mode

[이정현]

Hello, Dear Sami Tolvanen.
Thank you for reply.

> I agree that we shouldn't allow this, at least not without a warning, but
> out of curiosity, do you actually have a situation where this could happen?
> One ideally shouldn't be passing untrusted parameters to dm-verity.

Of course, I don't think this will happen because they are dm-verity experts.
But since we are humans, I think this case could happen accidentally.
So it would be a good at preventing these cases.

> I don't have a strong opinion about this, but the documentation doesn't
> talk about verity modes, so perhaps this could be reworded to something
> like "Conflicting error handling parameters"?

Yes of course. That looks better.

I also had some ambiguous about how to express it. 
This is because I couldn't find it in document. 
The code says verity mode, so I wrote it down. never mind it :) 

like this)
    case DM_VERITY_MODE_LOGGING:
    case DM_VERITY_MODE_RESTART:
    case DM_VERITY_MODE_PANIC:


> On Thu, Mar 11, 2021 at 4:19 AM JeongHyeon Lee <jhs2.lee@samsung.com>
> wrote:
> >
> > If there are multiple verity mode when parsing the verity mode of dm
> > verity table, it will be set as the last one.
> > So set to 'allow only once' to prevent it.
> 
> I agree that we shouldn't allow this, at least not without a warning, but
> out of curiosity, do you actually have a situation where this could happen?
> One ideally shouldn't be passing untrusted parameters to dm-verity.
> 
> >
> > Signed-off-by: JeongHyeon Lee <jhs2.lee@samsung.com>
> > ---
> >  drivers/md/dm-verity-target.c | 38
> > ++++++++++++++++++++++++++---------
> >  1 file changed, 28 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/md/dm-verity-target.c
> > b/drivers/md/dm-verity-target.c index 808a98ef624c..b76431dc7721
> > 100644
> > --- a/drivers/md/dm-verity-target.c
> > +++ b/drivers/md/dm-verity-target.c
> > @@ -893,6 +893,28 @@ static int verity_alloc_zero_digest(struct
> dm_verity *v)
> >         return r;
> >  }
> >
> > +static inline bool verity_is_verity_mode(const char *arg_name) {
> > +       return (!strcasecmp(arg_name, DM_VERITY_OPT_LOGGING) ||
> > +               !strcasecmp(arg_name, DM_VERITY_OPT_RESTART) ||
> > +               !strcasecmp(arg_name, DM_VERITY_OPT_PANIC)); }
> > +
> > +static int verity_parse_verity_mode(struct dm_verity *v, const char
> > +*arg_name) {
> > +       if (v->mode)
> > +               return -EINVAL;
> > +
> > +       if (!strcasecmp(arg_name, DM_VERITY_OPT_LOGGING))
> > +               v->mode = DM_VERITY_MODE_LOGGING;
> > +       else if (!strcasecmp(arg_name, DM_VERITY_OPT_RESTART))
> > +               v->mode = DM_VERITY_MODE_RESTART;
> > +       else if (!strcasecmp(arg_name, DM_VERITY_OPT_PANIC))
> > +               v->mode = DM_VERITY_MODE_PANIC;
> > +
> > +       return 0;
> > +}
> > +
> >  static int verity_parse_opt_args(struct dm_arg_set *as, struct
> dm_verity *v,
> >                                  struct dm_verity_sig_opts
> > *verify_args)  { @@ -916,16 +938,12 @@ static int
> > verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
> >                 arg_name = dm_shift_arg(as);
> >                 argc--;
> >
> > -               if (!strcasecmp(arg_name, DM_VERITY_OPT_LOGGING)) {
> > -                       v->mode = DM_VERITY_MODE_LOGGING;
> > -                       continue;
> > -
> > -               } else if (!strcasecmp(arg_name, DM_VERITY_OPT_RESTART)) {
> > -                       v->mode = DM_VERITY_MODE_RESTART;
> > -                       continue;
> > -
> > -               } else if (!strcasecmp(arg_name, DM_VERITY_OPT_PANIC)) {
> > -                       v->mode = DM_VERITY_MODE_PANIC;
> > +               if (verity_is_verity_mode(arg_name)) {
> > +                       r = verity_parse_verity_mode(v, arg_name);
> > +                       if (r) {
> > +                               ti->error = "Already verity mode set";
> 
> I don't have a strong opinion about this, but the documentation doesn't
> talk about verity modes, so perhaps this could be reworded to something
> like "Conflicting error handling parameters"?
> 
> > +                               return r;
> > +                       }
> >                         continue;
> >
> >                 } else if (!strcasecmp(arg_name,
> > DM_VERITY_OPT_IGN_ZEROES)) {
> > --
> > 2.17.1
> >
> 
> Sami