[PATCH 0/2] LUKS2 cleanups

[PATCH 0/2] LUKS2 cleanups

[Patrick Steinhardt]

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

Hi,

here's two small improvements for LUKS2 support:

- The first patch introduces better error reporting, as I've noticed
  it's quite hard to determine why decryption failed in case there's any
  misconfiguration.

- The second is a result of my upstream pull request in jsmn, which got
  accepted last week. As a result, we can now remove all the `jmntok_t`
  casts in our json library.

Patrick

Patrick Steinhardt (2):
  luks2: Improve error reporting when decrypting/verifying key
  json: Update jsmn library to get rid of casts

 grub-core/disk/luks2.c    | 8 +++++---
 grub-core/lib/json/jsmn.h | 7 +++++--
 grub-core/lib/json/json.c | 8 ++++----
 grub-core/lib/json/json.h | 4 +++-
 4 files changed, 17 insertions(+), 10 deletions(-)

-- 
2.26.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 1/2] luks2: Improve error reporting when decrypting/verifying key

[Patrick Steinhardt]

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

While we already set up error messages in both `luks2_verify_key()` and
`luks2_decrypt_key()`, we do not ever print them. This makes it really
hard to discover why a given key actually failed to decrypt a disk.

Improve this by including the error message in the user-visible output.
---
 grub-core/disk/luks2.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 65c4f0aac..58ac7bae1 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -487,7 +487,7 @@ luks2_decrypt_key (grub_uint8_t *out_key,
   ret = grub_disk_read (disk, 0, k->area.offset, k->area.size, split_key);
   if (ret)
     {
-      grub_dprintf ("luks2", "Read error: %s\n", grub_errmsg);
+      grub_error (GRUB_ERR_IO, "luks2", "Read error: %s\n", grub_errmsg);
       goto err;
     }
 
@@ -610,14 +610,16 @@ luks2_recover_key (grub_disk_t disk,
 			       (const grub_uint8_t *) passphrase, grub_strlen (passphrase));
       if (ret)
 	{
-	  grub_dprintf ("luks2", "Decryption with keyslot %"PRIuGRUB_SIZE" failed\n", i);
+	  grub_dprintf ("luks2", "Decryption with keyslot %"PRIuGRUB_SIZE" failed: %s\n",
+			i, grub_errmsg);
 	  continue;
 	}
 
       ret = luks2_verify_key (&digest, candidate_key, keyslot.key_size);
       if (ret)
 	{
-	  grub_dprintf ("luks2", "Could not open keyslot %"PRIuGRUB_SIZE"\n", i);
+	  grub_dprintf ("luks2", "Could not open keyslot %"PRIuGRUB_SIZE": %s\n",
+			i, grub_errmsg);
 	  continue;
 	}
 
-- 
2.26.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/2] json: Update jsmn library to get rid of casts

[Patrick Steinhardt]

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

Update our embedded version of the jsmn library to upstream commit
053d3cd (Merge pull request #175 from pks-t/pks/struct-type,
2020-04-02). The update adds a name for the `jsmntok` struct, which
allows us to add a forward declaration for the struct's typedef. As a
result, we can now convert the `void *` tokens member of `struct
grub_json` to `jsmntok_t *` and remove all casts.
---
 grub-core/lib/json/jsmn.h | 7 +++++--
 grub-core/lib/json/json.c | 8 ++++----
 grub-core/lib/json/json.h | 4 +++-
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/grub-core/lib/json/jsmn.h b/grub-core/lib/json/jsmn.h
index b95368a20..3178dcc97 100644
--- a/grub-core/lib/json/jsmn.h
+++ b/grub-core/lib/json/jsmn.h
@@ -66,7 +66,7 @@ enum jsmnerr {
  * start	start position in JSON data string
  * end		end position in JSON data string
  */
-typedef struct {
+typedef struct jsmntok {
   jsmntype_t type;
   int start;
   int end;
@@ -80,7 +80,7 @@ typedef struct {
  * JSON parser. Contains an array of token blocks available. Also stores
  * the string being parsed now and current position in that string.
  */
-typedef struct {
+typedef struct jsmn_parser {
   unsigned int pos;     /* offset in the JSON string */
   unsigned int toknext; /* next token to allocate */
   int toksuper;         /* superior token node, e.g. parent object or array */
@@ -154,6 +154,9 @@ static int jsmn_parse_primitive(jsmn_parser *parser, const char *js,
     case ']':
     case '}':
       goto found;
+    default:
+                   /* to quiet a warning from gcc*/
+      break;
     }
     if (js[parser->pos] < 32 || js[parser->pos] >= 127) {
       parser->pos = start;
diff --git a/grub-core/lib/json/json.c b/grub-core/lib/json/json.c
index 15c0d9949..694af4f3a 100644
--- a/grub-core/lib/json/json.c
+++ b/grub-core/lib/json/json.c
@@ -95,7 +95,7 @@ grub_json_getsize (grub_size_t *out, const grub_json_t *json)
 {
   int size;
 
-  size = ((jsmntok_t *)json->tokens)[json->idx].size;
+  size = json->tokens[json->idx].size;
   if (size < 0)
     return GRUB_ERR_OUT_OF_RANGE;
 
@@ -106,7 +106,7 @@ grub_json_getsize (grub_size_t *out, const grub_json_t *json)
 grub_err_t
 grub_json_gettype (grub_json_type_t *out, const grub_json_t *json)
 {
-  switch (((jsmntok_t *)json->tokens)[json->idx].type)
+  switch (json->tokens[json->idx].type)
     {
     case JSMN_OBJECT:
       *out = GRUB_JSON_OBJECT;
@@ -142,7 +142,7 @@ grub_json_getchild (grub_json_t *out, const grub_json_t *parent, grub_size_t n)
    * array), as well. We thus add the children's size to n on
    * each iteration.
    */
-  p = &((jsmntok_t *)parent->tokens)[parent->idx];
+  p = &parent->tokens[parent->idx];
   while (n--)
     n += p[offset++].size;
 
@@ -197,7 +197,7 @@ get_value (grub_json_type_t *out_type, const char **out_string, const grub_json_
       p = &child;
     }
 
-  tok = &((jsmntok_t *) p->tokens)[p->idx];
+  tok = &p->tokens[p->idx];
   p->string[tok->end] = '\0';
 
   *out_string = p->string + tok->start;
diff --git a/grub-core/lib/json/json.h b/grub-core/lib/json/json.h
index 358e4bca3..d9f99454d 100644
--- a/grub-core/lib/json/json.h
+++ b/grub-core/lib/json/json.h
@@ -36,9 +36,11 @@ enum grub_json_type
 };
 typedef enum grub_json_type grub_json_type_t;
 
+typedef struct jsmntok jsmntok_t;
+
 struct grub_json
 {
-  void	      *tokens;
+  jsmntok_t   *tokens;
   char	      *string;
   grub_size_t idx;
 };
-- 
2.26.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] luks2: Improve error reporting when decrypting/verifying key

[Daniel Kiper]

On Tue, Apr 07, 2020 at 06:02:23PM +0200, Patrick Steinhardt wrote:
> While we already set up error messages in both `luks2_verify_key()` and
> `luks2_decrypt_key()`, we do not ever print them. This makes it really
> hard to discover why a given key actually failed to decrypt a disk.
>
> Improve this by including the error message in the user-visible output.
> ---
>  grub-core/disk/luks2.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> index 65c4f0aac..58ac7bae1 100644
> --- a/grub-core/disk/luks2.c
> +++ b/grub-core/disk/luks2.c
> @@ -487,7 +487,7 @@ luks2_decrypt_key (grub_uint8_t *out_key,
>    ret = grub_disk_read (disk, 0, k->area.offset, k->area.size, split_key);
>    if (ret)
>      {
> -      grub_dprintf ("luks2", "Read error: %s\n", grub_errmsg);
> +      grub_error (GRUB_ERR_IO, "luks2", "Read error: %s\n", grub_errmsg);

I think that you should drop "luks2" here.

>        goto err;
>      }
>
> @@ -610,14 +610,16 @@ luks2_recover_key (grub_disk_t disk,
>  			       (const grub_uint8_t *) passphrase, grub_strlen (passphrase));
>        if (ret)
>  	{
> -	  grub_dprintf ("luks2", "Decryption with keyslot %"PRIuGRUB_SIZE" failed\n", i);
> +	  grub_dprintf ("luks2", "Decryption with keyslot %"PRIuGRUB_SIZE" failed: %s\n",
> +			i, grub_errmsg);
>  	  continue;
>  	}
>
>        ret = luks2_verify_key (&digest, candidate_key, keyslot.key_size);
>        if (ret)
>  	{
> -	  grub_dprintf ("luks2", "Could not open keyslot %"PRIuGRUB_SIZE"\n", i);
> +	  grub_dprintf ("luks2", "Could not open keyslot %"PRIuGRUB_SIZE": %s\n",
> +			i, grub_errmsg);

This messages will be printed only if debugging is enabled. Is it what
you expect?

Daniel

Re: [PATCH 2/2] json: Update jsmn library to get rid of casts

[Daniel Kiper]

On Tue, Apr 07, 2020 at 06:02:29PM +0200, Patrick Steinhardt wrote:
> Update our embedded version of the jsmn library to upstream commit
> 053d3cd (Merge pull request #175 from pks-t/pks/struct-type,
> 2020-04-02). The update adds a name for the `jsmntok` struct, which
> allows us to add a forward declaration for the struct's typedef. As a
> result, we can now convert the `void *` tokens member of `struct
> grub_json` to `jsmntok_t *` and remove all casts.

Missing SOB...

> ---
>  grub-core/lib/json/jsmn.h | 7 +++++--
>  grub-core/lib/json/json.c | 8 ++++----
>  grub-core/lib/json/json.h | 4 +++-
>  3 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/grub-core/lib/json/jsmn.h b/grub-core/lib/json/jsmn.h
> index b95368a20..3178dcc97 100644
> --- a/grub-core/lib/json/jsmn.h
> +++ b/grub-core/lib/json/jsmn.h
> @@ -66,7 +66,7 @@ enum jsmnerr {
>   * start	start position in JSON data string
>   * end		end position in JSON data string
>   */
> -typedef struct {
> +typedef struct jsmntok {
>    jsmntype_t type;
>    int start;
>    int end;
> @@ -80,7 +80,7 @@ typedef struct {
>   * JSON parser. Contains an array of token blocks available. Also stores
>   * the string being parsed now and current position in that string.
>   */
> -typedef struct {
> +typedef struct jsmn_parser {

Commit message says about jsmntok only. Here you add jsmn_parser too.
If it is needed then it should go into separate patch.

>    unsigned int pos;     /* offset in the JSON string */
>    unsigned int toknext; /* next token to allocate */
>    int toksuper;         /* superior token node, e.g. parent object or array */
> @@ -154,6 +154,9 @@ static int jsmn_parse_primitive(jsmn_parser *parser, const char *js,
>      case ']':
>      case '}':
>        goto found;
> +    default:
> +                   /* to quiet a warning from gcc*/
> +      break;

It seems to me that this belongs to separate patch.

>      }
>      if (js[parser->pos] < 32 || js[parser->pos] >= 127) {
>        parser->pos = start;
> diff --git a/grub-core/lib/json/json.c b/grub-core/lib/json/json.c
> index 15c0d9949..694af4f3a 100644
> --- a/grub-core/lib/json/json.c
> +++ b/grub-core/lib/json/json.c
> @@ -95,7 +95,7 @@ grub_json_getsize (grub_size_t *out, const grub_json_t *json)
>  {
>    int size;
>
> -  size = ((jsmntok_t *)json->tokens)[json->idx].size;
> +  size = json->tokens[json->idx].size;
>    if (size < 0)
>      return GRUB_ERR_OUT_OF_RANGE;
>
> @@ -106,7 +106,7 @@ grub_json_getsize (grub_size_t *out, const grub_json_t *json)
>  grub_err_t
>  grub_json_gettype (grub_json_type_t *out, const grub_json_t *json)
>  {
> -  switch (((jsmntok_t *)json->tokens)[json->idx].type)
> +  switch (json->tokens[json->idx].type)
>      {
>      case JSMN_OBJECT:
>        *out = GRUB_JSON_OBJECT;
> @@ -142,7 +142,7 @@ grub_json_getchild (grub_json_t *out, const grub_json_t *parent, grub_size_t n)
>     * array), as well. We thus add the children's size to n on
>     * each iteration.
>     */
> -  p = &((jsmntok_t *)parent->tokens)[parent->idx];
> +  p = &parent->tokens[parent->idx];
>    while (n--)
>      n += p[offset++].size;
>
> @@ -197,7 +197,7 @@ get_value (grub_json_type_t *out_type, const char **out_string, const grub_json_
>        p = &child;
>      }
>
> -  tok = &((jsmntok_t *) p->tokens)[p->idx];
> +  tok = &p->tokens[p->idx];
>    p->string[tok->end] = '\0';
>
>    *out_string = p->string + tok->start;
> diff --git a/grub-core/lib/json/json.h b/grub-core/lib/json/json.h
> index 358e4bca3..d9f99454d 100644
> --- a/grub-core/lib/json/json.h
> +++ b/grub-core/lib/json/json.h
> @@ -36,9 +36,11 @@ enum grub_json_type
>  };
>  typedef enum grub_json_type grub_json_type_t;
>
> +typedef struct jsmntok jsmntok_t;
> +
>  struct grub_json
>  {
> -  void	      *tokens;
> +  jsmntok_t   *tokens;
>    char	      *string;

Something is messed up with tabs and spaces here...

Daniel

Re: [PATCH 1/2] luks2: Improve error reporting when decrypting/verifying key

[Patrick Steinhardt]

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

On Tue, Apr 14, 2020 at 08:12:22PM +0200, Daniel Kiper wrote:
> On Tue, Apr 07, 2020 at 06:02:23PM +0200, Patrick Steinhardt wrote:
> > While we already set up error messages in both `luks2_verify_key()` and
> > `luks2_decrypt_key()`, we do not ever print them. This makes it really
> > hard to discover why a given key actually failed to decrypt a disk.
> >
> > Improve this by including the error message in the user-visible output.
> > ---
> >  grub-core/disk/luks2.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> > index 65c4f0aac..58ac7bae1 100644
> > --- a/grub-core/disk/luks2.c
> > +++ b/grub-core/disk/luks2.c
> > @@ -487,7 +487,7 @@ luks2_decrypt_key (grub_uint8_t *out_key,
> >    ret = grub_disk_read (disk, 0, k->area.offset, k->area.size, split_key);
> >    if (ret)
> >      {
> > -      grub_dprintf ("luks2", "Read error: %s\n", grub_errmsg);
> > +      grub_error (GRUB_ERR_IO, "luks2", "Read error: %s\n", grub_errmsg);
> 
> I think that you should drop "luks2" here.

Oops, yes, obviously.

> >        goto err;
> >      }
> >
> > @@ -610,14 +610,16 @@ luks2_recover_key (grub_disk_t disk,
> >  			       (const grub_uint8_t *) passphrase, grub_strlen (passphrase));
> >        if (ret)
> >  	{
> > -	  grub_dprintf ("luks2", "Decryption with keyslot %"PRIuGRUB_SIZE" failed\n", i);
> > +	  grub_dprintf ("luks2", "Decryption with keyslot %"PRIuGRUB_SIZE" failed: %s\n",
> > +			i, grub_errmsg);
> >  	  continue;
> >  	}
> >
> >        ret = luks2_verify_key (&digest, candidate_key, keyslot.key_size);
> >        if (ret)
> >  	{
> > -	  grub_dprintf ("luks2", "Could not open keyslot %"PRIuGRUB_SIZE"\n", i);
> > +	  grub_dprintf ("luks2", "Could not open keyslot %"PRIuGRUB_SIZE": %s\n",
> > +			i, grub_errmsg);
> 
> This messages will be printed only if debugging is enabled. Is it what
> you expect?

Well, I'm honestly not quite sure. The thing is that we potentially have
a number of different keyslots, not only one. So when we try keyslots in
turn, it's not unexpected for the first n-1 keyslots to not work if the
nth keyslot is the one we're trying to unlock.

The above is probably just an edge case as I expect most users to use a
single keyslot, only. And in that case it would probably we useful to
always print those messages, not only in case debugging is turned on.

What do you think?

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] json: Update jsmn library to get rid of casts

[Patrick Steinhardt]

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

On Tue, Apr 14, 2020 at 08:19:01PM +0200, Daniel Kiper wrote:
> On Tue, Apr 07, 2020 at 06:02:29PM +0200, Patrick Steinhardt wrote:
> > @@ -80,7 +80,7 @@ typedef struct {
> >   * JSON parser. Contains an array of token blocks available. Also stores
> >   * the string being parsed now and current position in that string.
> >   */
> > -typedef struct {
> > +typedef struct jsmn_parser {
> 
> Commit message says about jsmntok only. Here you add jsmn_parser too.
> If it is needed then it should go into separate patch.

Note that this is pulled in by the upgrade to the newer jsmn library,
bringing in not only the changes I want to make sure of.

> >    unsigned int pos;     /* offset in the JSON string */
> >    unsigned int toknext; /* next token to allocate */
> >    int toksuper;         /* superior token node, e.g. parent object or array */
> > @@ -154,6 +154,9 @@ static int jsmn_parse_primitive(jsmn_parser *parser, const char *js,
> >      case ']':
> >      case '}':
> >        goto found;
> > +    default:
> > +                   /* to quiet a warning from gcc*/
> > +      break;
> 
> It seems to me that this belongs to separate patch.

See above. This is brought in by the update to 053d3cd. I could split
this up into two parts, though. First the update of "jsmn.h", followed
by our own changes in our wrapper. That would probably clear things up?

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] json: Update jsmn library to get rid of casts

[Daniel Kiper]

On Wed, Apr 15, 2020 at 10:55:51PM +0200, Patrick Steinhardt wrote:
> On Tue, Apr 14, 2020 at 08:19:01PM +0200, Daniel Kiper wrote:
> > On Tue, Apr 07, 2020 at 06:02:29PM +0200, Patrick Steinhardt wrote:
> > > @@ -80,7 +80,7 @@ typedef struct {
> > >   * JSON parser. Contains an array of token blocks available. Also stores
> > >   * the string being parsed now and current position in that string.
> > >   */
> > > -typedef struct {
> > > +typedef struct jsmn_parser {
> >
> > Commit message says about jsmntok only. Here you add jsmn_parser too.
> > If it is needed then it should go into separate patch.
>
> Note that this is pulled in by the upgrade to the newer jsmn library,
> bringing in not only the changes I want to make sure of.
>
> > >    unsigned int pos;     /* offset in the JSON string */
> > >    unsigned int toknext; /* next token to allocate */
> > >    int toksuper;         /* superior token node, e.g. parent object or array */
> > > @@ -154,6 +154,9 @@ static int jsmn_parse_primitive(jsmn_parser *parser, const char *js,
> > >      case ']':
> > >      case '}':
> > >        goto found;
> > > +    default:
> > > +                   /* to quiet a warning from gcc*/
> > > +      break;
> >
> > It seems to me that this belongs to separate patch.
>
> See above. This is brought in by the update to 053d3cd. I could split
> this up into two parts, though. First the update of "jsmn.h", followed
> by our own changes in our wrapper. That would probably clear things up?

Yes, please split this patch into separate logical changes and post
as a separate patchset with cover letter saying that this aligns the
GRUB jsmn code with update 053d3cd.

Daniel

Re: [PATCH 1/2] luks2: Improve error reporting when decrypting/verifying key

[Daniel Kiper]

On Wed, Apr 15, 2020 at 10:52:53PM +0200, Patrick Steinhardt wrote:
> On Tue, Apr 14, 2020 at 08:12:22PM +0200, Daniel Kiper wrote:
> > On Tue, Apr 07, 2020 at 06:02:23PM +0200, Patrick Steinhardt wrote:
> > > While we already set up error messages in both `luks2_verify_key()` and
> > > `luks2_decrypt_key()`, we do not ever print them. This makes it really
> > > hard to discover why a given key actually failed to decrypt a disk.
> > >
> > > Improve this by including the error message in the user-visible output.
> > > ---
> > >  grub-core/disk/luks2.c | 8 +++++---
> > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> > > index 65c4f0aac..58ac7bae1 100644
> > > --- a/grub-core/disk/luks2.c
> > > +++ b/grub-core/disk/luks2.c
> > > @@ -487,7 +487,7 @@ luks2_decrypt_key (grub_uint8_t *out_key,
> > >    ret = grub_disk_read (disk, 0, k->area.offset, k->area.size, split_key);
> > >    if (ret)
> > >      {
> > > -      grub_dprintf ("luks2", "Read error: %s\n", grub_errmsg);
> > > +      grub_error (GRUB_ERR_IO, "luks2", "Read error: %s\n", grub_errmsg);
> >
> > I think that you should drop "luks2" here.
>
> Oops, yes, obviously.
>
> > >        goto err;
> > >      }
> > >
> > > @@ -610,14 +610,16 @@ luks2_recover_key (grub_disk_t disk,
> > >  			       (const grub_uint8_t *) passphrase, grub_strlen (passphrase));
> > >        if (ret)
> > >  	{
> > > -	  grub_dprintf ("luks2", "Decryption with keyslot %"PRIuGRUB_SIZE" failed\n", i);
> > > +	  grub_dprintf ("luks2", "Decryption with keyslot %"PRIuGRUB_SIZE" failed: %s\n",
> > > +			i, grub_errmsg);
> > >  	  continue;
> > >  	}
> > >
> > >        ret = luks2_verify_key (&digest, candidate_key, keyslot.key_size);
> > >        if (ret)
> > >  	{
> > > -	  grub_dprintf ("luks2", "Could not open keyslot %"PRIuGRUB_SIZE"\n", i);
> > > +	  grub_dprintf ("luks2", "Could not open keyslot %"PRIuGRUB_SIZE": %s\n",
> > > +			i, grub_errmsg);
> >
> > This messages will be printed only if debugging is enabled. Is it what
> > you expect?
>
> Well, I'm honestly not quite sure. The thing is that we potentially have
> a number of different keyslots, not only one. So when we try keyslots in
> turn, it's not unexpected for the first n-1 keyslots to not work if the
> nth keyslot is the one we're trying to unlock.
>
> The above is probably just an edge case as I expect most users to use a
> single keyslot, only. And in that case it would probably we useful to
> always print those messages, not only in case debugging is turned on.
>
> What do you think?

Should not we print an error unconditionally if all n slots failed?
Then user will know that decryption is not possible but will not be
bothered by subsequent keyslot failures. However, I would still print
error for each keyslot if debugging is enabled. Then if you have some
problems you can enable debugging and see which keyslots fail. Does it
make sense?

Daniel

Re: [PATCH 1/2] luks2: Improve error reporting when decrypting/verifying key

[Patrick Steinhardt]

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

On Thu, Apr 16, 2020 at 11:52:17AM +0200, Daniel Kiper wrote:
> On Wed, Apr 15, 2020 at 10:52:53PM +0200, Patrick Steinhardt wrote:
> > On Tue, Apr 14, 2020 at 08:12:22PM +0200, Daniel Kiper wrote:
> > > On Tue, Apr 07, 2020 at 06:02:23PM +0200, Patrick Steinhardt wrote:
> > > > While we already set up error messages in both `luks2_verify_key()` and
> > > > `luks2_decrypt_key()`, we do not ever print them. This makes it really
> > > > hard to discover why a given key actually failed to decrypt a disk.
> > > >
> > > > Improve this by including the error message in the user-visible output.
> > > > ---
> > > >  grub-core/disk/luks2.c | 8 +++++---
> > > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> > > > index 65c4f0aac..58ac7bae1 100644
> > > > --- a/grub-core/disk/luks2.c
> > > > +++ b/grub-core/disk/luks2.c
> > > > @@ -487,7 +487,7 @@ luks2_decrypt_key (grub_uint8_t *out_key,
> > > >    ret = grub_disk_read (disk, 0, k->area.offset, k->area.size, split_key);
> > > >    if (ret)
> > > >      {
> > > > -      grub_dprintf ("luks2", "Read error: %s\n", grub_errmsg);
> > > > +      grub_error (GRUB_ERR_IO, "luks2", "Read error: %s\n", grub_errmsg);
> > >
> > > I think that you should drop "luks2" here.
> >
> > Oops, yes, obviously.
> >
> > > >        goto err;
> > > >      }
> > > >
> > > > @@ -610,14 +610,16 @@ luks2_recover_key (grub_disk_t disk,
> > > >  			       (const grub_uint8_t *) passphrase, grub_strlen (passphrase));
> > > >        if (ret)
> > > >  	{
> > > > -	  grub_dprintf ("luks2", "Decryption with keyslot %"PRIuGRUB_SIZE" failed\n", i);
> > > > +	  grub_dprintf ("luks2", "Decryption with keyslot %"PRIuGRUB_SIZE" failed: %s\n",
> > > > +			i, grub_errmsg);
> > > >  	  continue;
> > > >  	}
> > > >
> > > >        ret = luks2_verify_key (&digest, candidate_key, keyslot.key_size);
> > > >        if (ret)
> > > >  	{
> > > > -	  grub_dprintf ("luks2", "Could not open keyslot %"PRIuGRUB_SIZE"\n", i);
> > > > +	  grub_dprintf ("luks2", "Could not open keyslot %"PRIuGRUB_SIZE": %s\n",
> > > > +			i, grub_errmsg);
> > >
> > > This messages will be printed only if debugging is enabled. Is it what
> > > you expect?
> >
> > Well, I'm honestly not quite sure. The thing is that we potentially have
> > a number of different keyslots, not only one. So when we try keyslots in
> > turn, it's not unexpected for the first n-1 keyslots to not work if the
> > nth keyslot is the one we're trying to unlock.
> >
> > The above is probably just an edge case as I expect most users to use a
> > single keyslot, only. And in that case it would probably we useful to
> > always print those messages, not only in case debugging is turned on.
> >
> > What do you think?
> 
> Should not we print an error unconditionally if all n slots failed?
> Then user will know that decryption is not possible but will not be
> bothered by subsequent keyslot failures. However, I would still print
> error for each keyslot if debugging is enabled. Then if you have some
> problems you can enable debugging and see which keyslots fail. Does it
> make sense?
> 
> Daniel

Okay, so that's exactly how it works right now: in case no slot
succeeds, we print "Invalid passphrase" to let the user know about it.
If he turns on debugging, he'll see why each of the individual keyslots
has failed, and this patch only increases the verbosity to actually
include useful information.

I'll resend this patch separate from the jsmn updates with the SOB and
the above fix for `grub_error()`.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]