Re: [PATCH 2/7] Cryptomount support key files

[TJ]

On 14/03/18 09:44, John Lane wrote:
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -949,6 +954,45 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int 
> argc, char **args)
>      hdr = NULL;
>  
>    have_it = 0;
> +  key = NULL;
> +
> +  if (state[4].set) /* Key file; fails back to passphrase entry */
> +    {
> +      grub_file_t keyfile;
> +      int keyfile_offset;
> +      grub_size_t requested_keyfile_size;
> +
> +      requested_keyfile_size = state[6].set ? grub_strtoul(state[6].arg, 0, 
> 0) : 0;
> +
> +      if (requested_keyfile_size > GRUB_CRYPTODISK_MAX_KEYFILE_SIZE)
> +        grub_printf (N_("Key file size exceeds maximum (%llu)\n"), \
> +                          (unsigned long long) 
> GRUB_CRYPTODISK_MAX_KEYFILE_SIZE);
> +      else
> +        {
> +          keyfile_offset = state[5].set ? grub_strtoul (state[5].arg, 0, 0) 
> : 0;
> +          keyfile_size = requested_keyfile_size ? requested_keyfile_size : \
> +                                          GRUB_CRYPTODISK_MAX_KEYFILE_SIZE;
> +
> +          keyfile = grub_file_open (state[4].arg);
> +          if (!keyfile)
> +            grub_printf (N_("Unable to open key file %s\n"), state[4].arg);
> +          else if (grub_file_seek (keyfile, keyfile_offset) == 
> (grub_off_t)-1)
> +            grub_printf (N_("Unable to seek to offset %d in key file\n"), 
> keyfile_offset);
> +          else
> +            {
> +              keyfile_size = grub_file_read (keyfile, keyfile_buffer, 
> keyfile_size);
> +              if (keyfile_size == (grub_size_t)-1)

grub_file_read() returns grub_ssize_t (signed). Is casting to
grub_size_t (unsigned) required or going to work as intended?

Is the only possible error -1? Underlying readwrite functions can return
error codes via grub_error() that are > 0: see include/grub/err.h