Re: GRUB release schedule?
Hi Bruce, On Sun, Oct 25, 2020 at 11:59:07AM -0500, Bruce Dubbs wrote: > Is there a release schedule for the next stable version of GRUB? It would > help for planning purposes. I am working on it now. I hope to cut rc1 in the following weeks and then release 2.06 in December. Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3 06/10] cryptodisk: Properly handle non-512 byte sized sectors.
On Fri, Oct 23, 2020 at 08:09:10PM +0200, Patrick Steinhardt wrote:
> On Mon, Oct 19, 2020 at 06:09:54PM -0500, Glenn Washburn wrote:
> > By default, dm-crypt internally uses an IV that corresponds to 512-byte
> > sectors, even when a larger sector size is specified. What this means is
> > that when using a larger sector size, the IV is incremented every sector.
> > However, the amount the IV is incremented is the number of 512 byte blocks
> > in a sector (ie 8 for 4K sectors). Confusingly the IV does not corespond to
> > the number of, for example, 4K sectors. So each 512 byte cipher block in a
> > sector will be encrypted with the same IV and the IV will be incremented
> > afterwards by the number of 512 byte cipher blocks in the sector.
> >
> > There are some encryption utilities which do it the intuitive way and have
> > the IV equal to the sector number regardless of sector size (ie. the fifth
> > sector would have an IV of 4 for each cipher block). And this is supported
> > by dm-crypt with the iv_large_sectors option and also cryptsetup as of 2.3.3
> > with the --iv-large-sectors, though not with LUKS headers (only with --type
> > plain). However, support for this has not been included as grub does not
> > support plain devices right now.
> >
> > One gotcha here is that the encrypted split keys are encrypted with a hard-
> > coded 512-byte sector size. So even if your data is encrypted with 4K sector
> > sizes, the split key encrypted area must be decrypted with a block size of
> > 512 (ie the IV increments every 512 bytes). This made these changes less
> > aestetically pleasing than desired.
> >
> > Signed-off-by: Glenn Washburn
> > ---
> > grub-core/disk/cryptodisk.c | 52 ++---
> > grub-core/disk/luks.c | 5 ++--
> > grub-core/disk/luks2.c | 7 -
> > include/grub/cryptodisk.h | 8 +-
> > 4 files changed, 47 insertions(+), 25 deletions(-)
> >
> > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > index a3d672f68..623f0f396 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -224,7 +224,8 @@ lrw_xor (const struct lrw_sector *sec,
> > static gcry_err_code_t
> > grub_cryptodisk_endecrypt (struct grub_cryptodisk *dev,
> >grub_uint8_t * data, grub_size_t len,
> > - grub_disk_addr_t sector, int do_encrypt)
> > + grub_disk_addr_t sector, grub_size_t log_sector_size,
> > + int do_encrypt)
> > {
> >grub_size_t i;
> >gcry_err_code_t err;
> > @@ -237,7 +238,7 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk *dev,
> > return (do_encrypt ? grub_crypto_ecb_encrypt (dev->cipher, data, data,
> > len)
> > : grub_crypto_ecb_decrypt (dev->cipher, data, data, len));
> >
> > - for (i = 0; i < len; i += (1U << dev->log_sector_size))
> > + for (i = 0; i < len; i += (1U << log_sector_size))
> > {
> >grub_size_t sz = ((dev->cipher->cipher->blocksize
> > + sizeof (grub_uint32_t) - 1)
> > @@ -270,7 +271,7 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk *dev,
> > if (!ctx)
> > return GPG_ERR_OUT_OF_MEMORY;
> >
> > - tmp = grub_cpu_to_le64 (sector << dev->log_sector_size);
> > + tmp = grub_cpu_to_le64 (sector << log_sector_size);
> > dev->iv_hash->init (ctx);
> > dev->iv_hash->write (ctx, dev->iv_prefix, dev->iv_prefix_len);
> > dev->iv_hash->write (ctx, &tmp, sizeof (tmp));
> > @@ -281,14 +282,23 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk
> > *dev,
> > }
> > break;
> > case GRUB_CRYPTODISK_MODE_IV_PLAIN64:
> > - iv[1] = grub_cpu_to_le32 (sector >> 32);
> > - /* FALLTHROUGH */
> > case GRUB_CRYPTODISK_MODE_IV_PLAIN:
> > - iv[0] = grub_cpu_to_le32 (sector & 0x);
> > + /*
> > + * The IV is a 32 or 64 bit value of the dm-crypt native sector
> > + * number. If using 32 bit IV mode, zero out the most significant
> > + * 32 bits.
> > + */
> > + {
> > + grub_uint64_t *iv64 = (grub_uint64_t *)iv;
> > + *iv64 = grub_cpu_to_le64 (sector << (log_sector_size
> > +-
> > GRUB_CRYPTODISK_IV_LOG_SIZE));
> > + if (dev->mode_iv == GRUB_CRYPTODISK_MODE_IV_PLAIN)
> > + iv[1] = 0;
> > + }
> > break;
> > case GRUB_CRYPTODISK_MODE_IV_BYTECOUNT64:
> > - iv[1] = grub_cpu_to_le32 (sector >> (32 - dev->log_sector_size));
> > - iv[0] = grub_cpu_to_le32 ((sector << dev->log_sector_size)
> > + iv[1] = grub_cpu_to_le32 (sector >> (32 - log_sector_size));
> > + iv[0] = grub_cpu_to_le32 ((sector << log_sector_size)
> > & 0x);
> > break;
> > case GRUB_CRYPTODISK_MODE_IV_BENBI:
> > @@ -311,10 +321,10 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk
> > *dev,
> > case GRUB_CRYPTODISK_MODE_CBC:
> >
Re: [PATCH v3 07/10] cryptodisk: Replace some literals with constants in grub_cryptodisk_endecrypt.
Oct 23, 2020 1:00:59 PM Patrick Steinhardt :
> On Mon, Oct 19, 2020 at 06:09:55PM -0500, Glenn Washburn wrote:
>> This should improve readability of code by providing clues as to what the
>> value represents.
>>
>> Signed-off-by: Glenn Washburn
>> ---
>> grub-core/disk/cryptodisk.c | 12 +++-
>> include/grub/types.h | 3 +++
>> 2 files changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
>> index 623f0f396..1a91c2d55 100644
>> --- a/grub-core/disk/cryptodisk.c
>> +++ b/grub-core/disk/cryptodisk.c
>> @@ -297,19 +297,21 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk *dev,
>> }
>> break;
>> case GRUB_CRYPTODISK_MODE_IV_BYTECOUNT64:
>> - iv[1] = grub_cpu_to_le32 (sector >> (32 - log_sector_size));
>> + /* The IV is the 64 bit byte offset of the sector. */
>> + iv[1] = grub_cpu_to_le32 (sector >> (GRUB_TYPE_BIT(iv[0])
>> + - log_sector_size));
>> iv[0] = grub_cpu_to_le32 ((sector << log_sector_size)
>> - & 0x);
>> + & GRUB_TYPE_MAX(iv[0]));
>> break;
>> case GRUB_CRYPTODISK_MODE_IV_BENBI:
>> {
>> grub_uint64_t num = (sector << dev->benbi_log) + 1;
>> - iv[sz - 2] = grub_cpu_to_be32 (num >> 32);
>> - iv[sz - 1] = grub_cpu_to_be32 (num & 0x);
>> + iv[sz - 2] = grub_cpu_to_be32 (num >> GRUB_TYPE_BIT(iv[0]));
>> + iv[sz - 1] = grub_cpu_to_be32 (num & GRUB_TYPE_MAX(iv[0]));
>> }
>> break;
>> case GRUB_CRYPTODISK_MODE_IV_ESSIV:
>> - iv[0] = grub_cpu_to_le32 (sector & 0x);
>> + iv[0] = grub_cpu_to_le32 (sector & GRUB_TYPE_MAX(iv[0]));
>> err = grub_crypto_ecb_encrypt (dev->essiv_cipher, iv, iv,
>> dev->cipher->cipher->blocksize);
>> if (err)
>> diff --git a/include/grub/types.h b/include/grub/types.h
>> index 035a4b528..8b4267ebd 100644
>> --- a/include/grub/types.h
>> +++ b/include/grub/types.h
>> @@ -319,4 +319,7 @@ static inline void grub_set_unaligned64 (void *ptr,
>> grub_uint64_t val)
>>
>> #define GRUB_CHAR_BIT 8
>>
>> +#define GRUB_TYPE_BIT(type) (sizeof(type) * GRUB_CHAR_BIT)
>
> Hum. I'd rather name this `GRUB_TYPE_BITS`, as the current name implies
> that we only get a single bit. Other than that the change looks good to
> me and I agree that it helps readability.
>
> Patrick
Yes, I would've liked to also, but I decided to stick with the naming
convention as can be seen in the definition of GRUB_CHAR_BIT above it. Should
there be a separate patch renaming GRUB_CHAR_BIT also?
>
>> +#define GRUB_TYPE_MAX(type) ((2 * ((1ULL << (GRUB_TYPE_BIT(type) - 1)) -
>> 1)) + 1)
>> +
>> #endif /* ! GRUB_TYPES_HEADER */
>> --
>> 2.27.0
>>
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2 7/8] efi: implement LoadFile2 initrd loading protocol for Linux
On Sun, Oct 25, 2020 at 6:50 AM Ard Biesheuvel wrote:
>
> Recent Linux kernels will invoke the LoadFile2 protocol installed on a
> well-known vendor media devicepath to load the initrd if it is exposed
> by the firmware. Using this method is preferred for two reasons:
> - the Linux kernel is in charge of allocating the memory, and so it can
> implement any placement policy it wants (given that these tend to
> change between kernel versions),
> - it is no longer necessary to modify the device tree provided by the
> firmware.
>
> So let's install this protocol when handling the 'initrd' command if
> such a recent kernel was detected (based on the PE/COFF image version),
> and defer loading the initrd contents until the point where the kernel
> invokes the LoadFile2 protocol.
>
Thanks for adding the support for LoadFile2 protocol. This was one of
the blockers for full RISC-V support.
How do you prefer to proceed with RISC-V support ?
For RISC-V, we just need to move the arm64 loader to a common file so
that both RISC-V/ARM64 can use it apart from
few header file fixes. During the last attempt[1], I moved it to
loader/efi/linux.c.
I am happy to test it on Qemu/hardware, if you want to send the series
either part of this one or a separate series.
I can rebase my previous series on top of this series as well. Please
let me know your preference.
[1] https://www.mail-archive.com/grub-devel@gnu.org/msg30107.html
> Signed-off-by: Ard Biesheuvel
> ---
> grub-core/loader/arm64/linux.c | 109 +++-
> 1 file changed, 108 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
> index 28ff8584a3b5..c6a95e1f0c43 100644
> --- a/grub-core/loader/arm64/linux.c
> +++ b/grub-core/loader/arm64/linux.c
> @@ -48,6 +48,10 @@ static grub_uint32_t cmdline_size;
> static grub_addr_t initrd_start;
> static grub_addr_t initrd_end;
>
> +static struct grub_linux_initrd_context initrd_ctx = { 0, 0, 0 };
> +static grub_efi_handle_t initrd_lf2_handle;
> +static int initrd_use_loadfile2;
> +
> grub_err_t
> grub_arch_efi_linux_load_image_header (grub_file_t file,
>struct linux_arch_kernel_header * lh)
> @@ -81,6 +85,18 @@ grub_arch_efi_linux_load_image_header (grub_file_t file,
> return grub_error(GRUB_ERR_FILE_READ_ERROR, "failed to read COFF
> image header");
> }
>
> + /*
> + * Linux kernels built for any architecture are guaranteed to support the
> + * LoadFile2 based initrd loading protocol if the image version is >= 1.
> + */
> + if (lh->coff_image_header.optional_header.major_image_version >= 1)
> +initrd_use_loadfile2 = 1;
> + else
> +initrd_use_loadfile2 = 0;
> +
> + grub_dprintf ("linux", "LoadFile2 initrd loading %sabled\n",
> + initrd_use_loadfile2 ? "en" : "dis");
> +
>return GRUB_ERR_NONE;
> }
>
> @@ -250,13 +266,86 @@ allocate_initrd_mem (int initrd_pages)
>GRUB_EFI_LOADER_DATA);
> }
>
> +struct initrd_media_device_path {
> + grub_efi_vendor_media_device_path_t vendor;
> + grub_efi_device_path_t end;
> +} GRUB_PACKED;
> +
> +#define LINUX_EFI_INITRD_MEDIA_GUID \
> + { 0x5568e427, 0x68fc, 0x4f3d, \
> +{ 0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68 } \
> + }
> +
> +static struct initrd_media_device_path initrd_lf2_device_path = {
> + {
> +{
> + GRUB_EFI_MEDIA_DEVICE_PATH_TYPE,
> + GRUB_EFI_VENDOR_MEDIA_DEVICE_PATH_SUBTYPE,
> + sizeof(grub_efi_vendor_media_device_path_t),
> +},
> +LINUX_EFI_INITRD_MEDIA_GUID
> + }, {
> +GRUB_EFI_END_DEVICE_PATH_TYPE,
> +GRUB_EFI_END_ENTIRE_DEVICE_PATH_SUBTYPE,
> +sizeof(grub_efi_device_path_t)
> + }
> +};
> +
> +static grub_efi_status_t initrd_load_file2(grub_efi_load_file2_t *this,
> + grub_efi_device_path_t
> *device_path,
> + grub_efi_boolean_t boot_policy,
> + grub_efi_uintn_t *buffer_size,
> + void *buffer);
> +
> +static grub_efi_load_file2_t initrd_lf2 = {
> + initrd_load_file2
> +};
> +
> +static grub_efi_status_t initrd_load_file2(grub_efi_load_file2_t *this,
> + grub_efi_device_path_t
> *device_path,
> + grub_efi_boolean_t boot_policy,
> + grub_efi_uintn_t *buffer_size,
> + void *buffer)
> +{
> + grub_efi_status_t status = GRUB_EFI_SUCCESS;
> + grub_efi_uintn_t initrd_size;
> +
> + if (!this || this != &initrd_lf2 || !buffer_size)
> +return GRUB_EFI_INVALID_PARAMETER;
> +
> + if (device_path->type != GRUB_EFI_END_DEVICE_PATH_TYPE ||
> + device_path->subtype != GRUB_EFI_END_ENTIRE_DEVICE_PATH_SUBTYPE)
> +return GRUB_EFI_NOT_FOUND;
> +
> + if (boot_policy)
> +ret
Re: [PATCH v2 7/8] efi: implement LoadFile2 initrd loading protocol for Linux
On Mon, 26 Oct 2020 at 22:38, Atish Patra wrote:
>
> On Sun, Oct 25, 2020 at 6:50 AM Ard Biesheuvel wrote:
> >
> > Recent Linux kernels will invoke the LoadFile2 protocol installed on a
> > well-known vendor media devicepath to load the initrd if it is exposed
> > by the firmware. Using this method is preferred for two reasons:
> > - the Linux kernel is in charge of allocating the memory, and so it can
> > implement any placement policy it wants (given that these tend to
> > change between kernel versions),
> > - it is no longer necessary to modify the device tree provided by the
> > firmware.
> >
> > So let's install this protocol when handling the 'initrd' command if
> > such a recent kernel was detected (based on the PE/COFF image version),
> > and defer loading the initrd contents until the point where the kernel
> > invokes the LoadFile2 protocol.
> >
>
> Thanks for adding the support for LoadFile2 protocol. This was one of
> the blockers for full RISC-V support.
> How do you prefer to proceed with RISC-V support ?
>
I'll defer to Daniel and Leif for the answer to that question.
> For RISC-V, we just need to move the arm64 loader to a common file so
> that both RISC-V/ARM64 can use it apart from
> few header file fixes. During the last attempt[1], I moved it to
> loader/efi/linux.c.
>
That seems the most logical to me. Given that it is shared between ARM
and arm64 today, it doesn't belong in grub-core/loader/arm64/linux.c
to begin with.
As I understand it, Daniel is doing a release imminently. Once that is
out of the way, I'm happy to proceed whichever way the maintainers
prefer.
> I am happy to test it on Qemu/hardware, if you want to send the series
> either part of this one or a separate series.
> I can rebase my previous series on top of this series as well. Please
> let me know your preference.
>
That would be a good start in any case, to ensure that the PE/COFF
image loading and LoadFile2 handling works as expected. And perhaps
simply merging the changes in that order is the most straight-forward.
> [1] https://www.mail-archive.com/grub-devel@gnu.org/msg30107.html
>
> > Signed-off-by: Ard Biesheuvel
> > ---
> > grub-core/loader/arm64/linux.c | 109 +++-
> > 1 file changed, 108 insertions(+), 1 deletion(-)
> >
> > diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
> > index 28ff8584a3b5..c6a95e1f0c43 100644
> > --- a/grub-core/loader/arm64/linux.c
> > +++ b/grub-core/loader/arm64/linux.c
> > @@ -48,6 +48,10 @@ static grub_uint32_t cmdline_size;
> > static grub_addr_t initrd_start;
> > static grub_addr_t initrd_end;
> >
> > +static struct grub_linux_initrd_context initrd_ctx = { 0, 0, 0 };
> > +static grub_efi_handle_t initrd_lf2_handle;
> > +static int initrd_use_loadfile2;
> > +
> > grub_err_t
> > grub_arch_efi_linux_load_image_header (grub_file_t file,
> >struct linux_arch_kernel_header * lh)
> > @@ -81,6 +85,18 @@ grub_arch_efi_linux_load_image_header (grub_file_t file,
> > return grub_error(GRUB_ERR_FILE_READ_ERROR, "failed to read COFF
> > image header");
> > }
> >
> > + /*
> > + * Linux kernels built for any architecture are guaranteed to support the
> > + * LoadFile2 based initrd loading protocol if the image version is >= 1.
> > + */
> > + if (lh->coff_image_header.optional_header.major_image_version >= 1)
> > +initrd_use_loadfile2 = 1;
> > + else
> > +initrd_use_loadfile2 = 0;
> > +
> > + grub_dprintf ("linux", "LoadFile2 initrd loading %sabled\n",
> > + initrd_use_loadfile2 ? "en" : "dis");
> > +
> >return GRUB_ERR_NONE;
> > }
> >
> > @@ -250,13 +266,86 @@ allocate_initrd_mem (int initrd_pages)
> >GRUB_EFI_LOADER_DATA);
> > }
> >
> > +struct initrd_media_device_path {
> > + grub_efi_vendor_media_device_path_t vendor;
> > + grub_efi_device_path_t end;
> > +} GRUB_PACKED;
> > +
> > +#define LINUX_EFI_INITRD_MEDIA_GUID \
> > + { 0x5568e427, 0x68fc, 0x4f3d, \
> > +{ 0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68 } \
> > + }
> > +
> > +static struct initrd_media_device_path initrd_lf2_device_path = {
> > + {
> > +{
> > + GRUB_EFI_MEDIA_DEVICE_PATH_TYPE,
> > + GRUB_EFI_VENDOR_MEDIA_DEVICE_PATH_SUBTYPE,
> > + sizeof(grub_efi_vendor_media_device_path_t),
> > +},
> > +LINUX_EFI_INITRD_MEDIA_GUID
> > + }, {
> > +GRUB_EFI_END_DEVICE_PATH_TYPE,
> > +GRUB_EFI_END_ENTIRE_DEVICE_PATH_SUBTYPE,
> > +sizeof(grub_efi_device_path_t)
> > + }
> > +};
> > +
> > +static grub_efi_status_t initrd_load_file2(grub_efi_load_file2_t *this,
> > + grub_efi_device_path_t
> > *device_path,
> > + grub_efi_boolean_t boot_policy,
> > + grub_efi_uintn_t *buffer_size,
> > + void *buffer);
> > +
> > +stati
