RE: [PATCH 2/2] dm verity: allow only one verify mode
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 > 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 > > --- > > 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 > 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 > > --- > > 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
On Thu, Mar 11, 2021 at 4:19 AM JeongHyeon Lee 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 > --- > 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