Re: [PATCHv3] hurd: Support device entries with @/dev/disk: qualifier
On Wed, Apr 27, 2022 at 11:00:29PM +0200, Samuel Thibault wrote:
> Those are used with non-bootstrap disk drivers, for which libstore has to
> open /dev/disk before calling device_open on it instead of on the device
> master port. Normally in that case all /dev/ entries also have the
> @/dev/disk:
> qualifier, so we can just drop it.
>
> Signed-off-by: Samuel Thibault
>
> Message-Id: <20220223233413.wkk66pxp5p2q2wrf@begin>
>
> ---
> Difference with v2: formatting, using xmalloc instead of malloc.
>
> Difference with v1: better drop the @/dev/disk: qualifier right from
> grub_util_hurd_get_disk_info so it benefits alls the callees and not
> only grub_util_part_to_disk.
>
> diff --git a/grub-core/osdep/hurd/getroot.c b/grub-core/osdep/hurd/getroot.c
> index c66b206fa..5f0e366d1 100644
> --- a/grub-core/osdep/hurd/getroot.c
> +++ b/grub-core/osdep/hurd/getroot.c
> @@ -112,9 +112,21 @@ grub_util_find_hurd_root_device (const char *path)
>if (strncmp (name, "device:", sizeof ("device:") - 1) == 0)
> {
>char *dev_name = name + sizeof ("device:") - 1;
> - size_t size = sizeof ("/dev/") - 1 + strlen (dev_name) + 1;
> + size_t size;
>char *next;
> - ret = malloc (size);
> +
> + if (dev_name[0] == '@')
> +{
> + /* non-bootstrap disk driver, the /dev/ entry is normally set up
> with
> + * the same @. */
> + char *next_name = strchr (dev_name, ':');
> +
> + if (next_name)
> +dev_name = next_name + 1;
> +}
> +
> + size = sizeof ("/dev/") - 1 + strlen (dev_name) + 1;
> + ret = xmalloc (size);
>next = stpncpy (ret, "/dev/", size);
>stpncpy (next, dev_name, size - (next - ret));
> }
> diff --git a/grub-core/osdep/hurd/hostdisk.c b/grub-core/osdep/hurd/hostdisk.c
> index c47b5a5ea..73c442ae5 100644
> --- a/grub-core/osdep/hurd/hostdisk.c
> +++ b/grub-core/osdep/hurd/hostdisk.c
> @@ -87,6 +87,21 @@ grub_util_hurd_get_disk_info (const char *dev,
> grub_uint32_t *secsize, grub_disk
> *parent = xmalloc (len+1);
> memcpy (*parent, data, len);
> (*parent)[len] = '\0';
> +
> + if ((*parent)[0] == '@')
> + {
> + /* non-bootstrap disk driver, the /dev/ entry is normally set up
> with
> +* the same @. */
> + char *next_path = strchr (*parent, ':');
> +
> + if (next_path)
> + {
> + char *n = strdup (next_path + 1);
I think this should be xstrdup() instead of strdup(). I can fix this for
you before push.
Otherwise Reviewed-by: Daniel Kiper
Daniel
> + free (*parent);
> + *parent = n;
> + }
> + }
> }
> }
>if (offset)
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3 15/19] appended signatures: parse PKCS#7 signedData and X.509 certificates
Hi Michael, apologies for the delay, I've left IBM and am taking a while to get back on my feet. > On Thu, Apr 21, 2022 at 09:32:41PM +1000, Daniel Axtens wrote: >> Hi, >> >> >> This code allows us to parse: >> >> >> >> - PKCS#7 signedData messages. Only a single signerInfo is supported, >> >>which is all that the Linux sign-file utility supports creating >> >>out-of-the-box. Only RSA, SHA-256 and SHA-512 are supported. >> >>Any certificate embedded in the PKCS#7 message will be ignored. >> >> >> >> - X.509 certificates: at least enough to verify the signatures on the >> >>PKCS#7 messages. We expect that the certificates embedded in grub will >> >>be leaf certificates, not CA certificates. The parser enforces this. >> > >> > Doesn't grub support CA certificates for EFI? >> > >> For EFI, the verification isn't done by grub, it's done by either the >> shim or by firmware. They do - per the spec - support certificate >> chaining. So you could validly put a CA cert into db or MokList and have >> a signature on an Authenticode signature chain back to that. >> >> > Why limit to leaf certificates here? >> >> Conventionally, appended signatures don't include embedded certificates >> whereas Authenticode signatures do. There's nothing stopping you putting >> one into the PKCS#7 message for an appended signature - it's just not >> the way they are used on kernel modules or kernel binaries. So if >> there's not an embedded certificate that can chain back to a CA >> certificate, grub has to have access to the leaf certificate. >> >> > If this is technical limitation of the code in question could you fail >> > the build when CA certificate is used rather than crashing the >> > bootloader when it boots? >> >> I guess. I think IBM has been reasonably open in saying that static key >> secure boot is not the end of the road for Power, so we expect that >> ultimately keys could be loaded from somewhere other than the binary >> like firmware storage. So even if we added this check in grub-mkimage, >> we'd still need to keep it at runtime as well. >> >> >> - X.509 certificates support the Extended Key Usage extension and handle >> >>it by verifying that the certificate has a single purpose, that is code >> > ^^ >> > This should be updated I suppose. >> >> So this is - surprisingly - still accurate! We still only support 1 >> _Extended_ Key Usage. The change that we made as a result of SUSE's bug > > It is unclear to me OpenFirmware enforces such restriction as well ? My > understanding is that the (single purpose) restriction is lifed in > firmware becuase it happily accepts our cert and boots SUSE signed grub > in secure boot without complaining Key Usage. If so I wonder why it is > necessary for having the stricter check in grub ? I suppose it depends on the particular OpenFirmware implementation. You would have to ask IBM via official channels what restrictions the proprietary firmware imposes. The SLOF prototype probably imposes no restrictions because it was thrown together very very quickly as a proof of concept and development tool. I do still want to tease out (or repeat ad nauseum) the difference between Key Usage and Extended Key Usage. The original code permitted a Key Usage of only Digitial Signature and no critical Extended Key Usages. Subsequent versions, such as this one, support multiple Key Usages, so long as the Digital Signature Usage is present, and either zero or one Extended Key Usage sections, which permit only the Code Signing EKU. AIUI/IIRC, SUSE OBS keys have Digital Signature and another Key Usage set - which was where we ran into issues with your keys. >> report was to support additional regular, non-extended key >> usages. Here's the change broken out: > > Would this change be used next version ? > The next proprietary firmware version, or the next revision of this series? I don't have any insights into the future firmware development. For grub, that change is part of this v3 series already. Kind regards, Daniel > Thanks, > Michael > >> >> commit 6056d5fc59907c486309c401135757f00ebf7760 >> Author: Daniel Axtens >> Date: Tue Nov 30 15:00:57 2021 +1100 >> >> x509: allow Digitial Signature plus other Key Usages >> >> Currently the x509 certificate parser for appended signature >> verification requires that the certificate have the Digitial Signature >> key usage and _only_ the Digitial Signature use. This is overly strict >> and becomes policy enforcement rather than a security property. >> >> Require that the Digitial Signature usage is present, but do not >> require that it is the only usage present. >> >> Reported-by: Michal Suchanek >> Signed-off-by: Daniel Axtens >> >> diff --git a/grub-core/commands/appendedsig/x509.c >> b/grub-core/commands/appendedsig/x509.c >> index 70480aa73c9d..6ae985b30ff8 100644 >> --- a/grub-core/commands/ap
Re: [PATCH 0/2] powerpc-ieee1275: support larger core.elf images
Hi, Sorry, I missed this as well - my otherwise lovely threaded email client doesn't sort on the basis of most recent message, but thread start date. > root@ibook-g4-14:/home/glaubitz/grub# grub-install -d ./grub-core > --macppc-directory=/boot/grub --no-nvram > Installing for powerpc-ieee1275 platform. > grub-install: warning: cannot open directory `/usr/local/share/locale': No > such file or directory. > Installation finished. No error reported. > root@ibook-g4-14:/home/glaubitz/grub# > > However, after rebooting the machine, the machine is now unbootable. The > firmware > is unable to even load GRUB. I'm just getting the question mark sign which > indicates > that the firmware cannot find the bootloader. That's really irritating. (and apologies for rendering your machine unbootable!) I don't have a good idea what the cause of that would be. I don't think I saw anything similar under qemu's mac99 model. Is there any chance you could dump the `available` property of the /memory node before grub is loaded? I don't think it's likely to be very revealing but it's my best idea so far... I've also since left IBM so I'm not likely to have a heap of time to follow it up, unfortnately. The patch does solve a real issue on Power systems, so I guess the next best thing if we can't root-cause it is to make the link address compile-time configurable :/ Kind regards, Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCHv3] hurd: Support device entries with @/dev/disk: qualifier
Daniel Kiper, le mar. 17 mai 2022 16:22:10 +0200, a ecrit:
> On Wed, Apr 27, 2022 at 11:00:29PM +0200, Samuel Thibault wrote:
> > Those are used with non-bootstrap disk drivers, for which libstore has to
> > open /dev/disk before calling device_open on it instead of on the device
> > master port. Normally in that case all /dev/ entries also have the
> > @/dev/disk:
> > qualifier, so we can just drop it.
> >
> > Signed-off-by: Samuel Thibault
> >
> > Message-Id: <20220223233413.wkk66pxp5p2q2wrf@begin>
> >
> > ---
> > Difference with v2: formatting, using xmalloc instead of malloc.
> >
> > Difference with v1: better drop the @/dev/disk: qualifier right from
> > grub_util_hurd_get_disk_info so it benefits alls the callees and not
> > only grub_util_part_to_disk.
> >
> > diff --git a/grub-core/osdep/hurd/getroot.c b/grub-core/osdep/hurd/getroot.c
> > index c66b206fa..5f0e366d1 100644
> > --- a/grub-core/osdep/hurd/getroot.c
> > +++ b/grub-core/osdep/hurd/getroot.c
> > @@ -112,9 +112,21 @@ grub_util_find_hurd_root_device (const char *path)
> >if (strncmp (name, "device:", sizeof ("device:") - 1) == 0)
> > {
> >char *dev_name = name + sizeof ("device:") - 1;
> > - size_t size = sizeof ("/dev/") - 1 + strlen (dev_name) + 1;
> > + size_t size;
> >char *next;
> > - ret = malloc (size);
> > +
> > + if (dev_name[0] == '@')
> > +{
> > + /* non-bootstrap disk driver, the /dev/ entry is normally set up
> > with
> > + * the same @. */
> > + char *next_name = strchr (dev_name, ':');
> > +
> > + if (next_name)
> > +dev_name = next_name + 1;
> > +}
> > +
> > + size = sizeof ("/dev/") - 1 + strlen (dev_name) + 1;
> > + ret = xmalloc (size);
> >next = stpncpy (ret, "/dev/", size);
> >stpncpy (next, dev_name, size - (next - ret));
> > }
> > diff --git a/grub-core/osdep/hurd/hostdisk.c
> > b/grub-core/osdep/hurd/hostdisk.c
> > index c47b5a5ea..73c442ae5 100644
> > --- a/grub-core/osdep/hurd/hostdisk.c
> > +++ b/grub-core/osdep/hurd/hostdisk.c
> > @@ -87,6 +87,21 @@ grub_util_hurd_get_disk_info (const char *dev,
> > grub_uint32_t *secsize, grub_disk
> > *parent = xmalloc (len+1);
> > memcpy (*parent, data, len);
> > (*parent)[len] = '\0';
> > +
> > + if ((*parent)[0] == '@')
> > + {
> > + /* non-bootstrap disk driver, the /dev/ entry is normally set up
> > with
> > + * the same @. */
> > + char *next_path = strchr (*parent, ':');
> > +
> > + if (next_path)
> > + {
> > + char *n = strdup (next_path + 1);
>
> I think this should be xstrdup() instead of strdup(). I can fix this for
> you before push.
Ok, please do so :)
Samuel
> Otherwise Reviewed-by: Daniel Kiper
>
> Daniel
>
> > + free (*parent);
> > + *parent = n;
> > + }
> > + }
> > }
> > }
> >if (offset)
>
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] hurd: Use part: qualifier
On Sat, Feb 26, 2022 at 12:51:18PM +0100, Samuel Thibault wrote: > When using userland drivers such as rumpdisk, we'd rather make ext2fs use > parted-based libstore partitioning support. That can be used for kernelland > drivers as well, so we can just make grub always use the part: qualifier to > switch ext2fs to it. > > grub_util_find_hurd_root_device then has to understand this syntax and > translate > it into the /dev/ entry name. > > Signed-off-by: Samuel Thibault Reviewed-by: Daniel Kiper Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] net: Fix incorrect condition for calling grub_net_tcp_retransmit()
On Wed, May 11, 2022 at 09:44:01PM -0500, Glenn Washburn wrote: > The commit 848724273e4 (net/net: Avoid unnecessary calls to > grub_net_tcp_retransmit()) needs to have its condition inverted to avoid > unnecessary calls to grub_net_tcp_retransmit(). As it is, it creates many > unnecessary calls and does not call grub_net_tcp_retransmit() when needed. > The call to grub_net_tcp_retransmit() should only be made when > grub_net_cards does _not_ equal NULL, meaning that there are potentially > network cards that need tcp retransmission. > > Signed-off-by: Glenn Washburn Reviewed-by: Daniel Kiper Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2] efidisk: pass buffers with higher alignment
On Tue, May 10, 2022 at 10:40:15PM +0200, Stefan Agner wrote:
> On 2022-05-10 22:26, Heinrich Schuchardt wrote:
> > On 5/10/22 21:59, Stefan Agner wrote:
> >> Despite the UEFI specification saying "the requirement is that the
> >> start address of a buffer must be evenly divisible by IoAlign with
> >> no remainder.", it seems that a higher alignment requirement is
> >> neecssary on some system (e.g. a Intel NUC system with NVMe SSD).
> >> That particular system has IoAlign set to 2, and sometimes returns
> >> status 7 when buffers with alignment of 2 are passed. Things seem
> >> to work fine with buffers aligned to 4 bytes.
> >>
> >> It seems that some system interpret IoAlign > 1 to mean 2 ^ IoAlign.
> >> There is also such a hint in an example printed in the Driver Writer's
> >> Guide:
> >> ScsiPassThruMode.IoAlign = 2; // Data must be alligned on 4-byte boundary
I think this confusion comes from improper interpretation of this part
of UEFI spec: IoAlign Supplies the alignment requirement for any buffer
used in a data transfer. IoAlign values of 0 and 1 mean that the buffer
can be placed anywhere in memory. Otherwise, IoAlign must be a power of
2, and...
I think it would be nice to point that out in the commit message.
> >> However, some systems (e.g. U-Boot and some drivers in EDK II) do follow
> >> the UEFI specification.
> >>
> >> Work around by using an alignment of at least 512 bytes in case
When you look at the fix it is not obvious. So, I would update this
sentence. Hmmm... I would prefer to drop 512 number from here
> >> alignment is requested. Also make sure that IoAlign is still respected
> >> as per UEFI specification if a higher alignment than block size is
> >> requested.
Could you explain how did you come up with this algorithm? Why did not
you use 512 number directly as lower limit as Heinrich suggested? And
your algorithm will not work for buggy UEFI implementations which has
IoAlign > 9. Do we care?
> >> Note: The problem has only noticed with compressed squashfs. It seems
> >> that ext4 (and presumably other file system drivers) pass buffers with
> >> a higher alignment already.
> >>
> >> Signed-off-by: Stefan Agner
> >> ---
> >> grub-core/disk/efi/efidisk.c | 18 --
> >> 1 file changed, 16 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/grub-core/disk/efi/efidisk.c b/grub-core/disk/efi/efidisk.c
> >> index 562543b35..dec4e2e8f 100644
> >> --- a/grub-core/disk/efi/efidisk.c
> >> +++ b/grub-core/disk/efi/efidisk.c
> >> @@ -553,8 +553,22 @@ grub_efidisk_readwrite (struct grub_disk *disk,
> >> grub_disk_addr_t sector,
> >> d = disk->data;
> >> bio = d->block_io;
> >>
> >> - /* Set alignment to 1 if 0 specified */
> >> - io_align = bio->media->io_align ? bio->media->io_align : 1;
> >> + /*
> >> + * If IoAlign is > 1, it should represent the required alignment.
> >> However,
> >> + * some UEFI implementation on Intel NUC systems seem to use IoAlign=2
> >> but
> >> + * require 2^IoAlign.
> >> + * Make sure to align to at least block size if IO alignment is
> >> required.
> >> + */
> >> + if (bio->media->io_align > 1)
> >> +{
> >> + if (bio->media->io_align < bio->media->block_size)
> >
> > block_size is not required to be a power of two (though it typically
> > is). But io_align should be a power of two. So here some logic is
> > missing. E.g.
> >
> > if (bio->media->io_align < bio->media->block_size &&
> > !(bio->media->block_size & (bio->media->block_size - 1))
> >
> > Or simply use a fixed value 512 as lower limit.
>
> This code in grub_efidisk_open should take care of that:
>
> if (m->block_size & (m->block_size - 1) || !m->block_size)
> return grub_error (GRUB_ERR_IO, "invalid sector size %d",
> m->block_size);
This should be mentioned in the commit message.
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2] efidisk: pass buffers with higher alignment
On Tue, May 17, 2022 at 05:07:49PM +0200, Daniel Kiper wrote:
> On Tue, May 10, 2022 at 10:40:15PM +0200, Stefan Agner wrote:
> > On 2022-05-10 22:26, Heinrich Schuchardt wrote:
> > > On 5/10/22 21:59, Stefan Agner wrote:
> > >> Despite the UEFI specification saying "the requirement is that the
> > >> start address of a buffer must be evenly divisible by IoAlign with
> > >> no remainder.", it seems that a higher alignment requirement is
> > >> neecssary on some system (e.g. a Intel NUC system with NVMe SSD).
> > >> That particular system has IoAlign set to 2, and sometimes returns
> > >> status 7 when buffers with alignment of 2 are passed. Things seem
> > >> to work fine with buffers aligned to 4 bytes.
> > >>
> > >> It seems that some system interpret IoAlign > 1 to mean 2 ^ IoAlign.
> > >> There is also such a hint in an example printed in the Driver Writer's
> > >> Guide:
> > >> ScsiPassThruMode.IoAlign = 2; // Data must be alligned on 4-byte boundary
>
> I think this confusion comes from improper interpretation of this part
> of UEFI spec: IoAlign Supplies the alignment requirement for any buffer
> used in a data transfer. IoAlign values of 0 and 1 mean that the buffer
> can be placed anywhere in memory. Otherwise, IoAlign must be a power of
> 2, and...
>
> I think it would be nice to point that out in the commit message.
>
> > >> However, some systems (e.g. U-Boot and some drivers in EDK II) do follow
> > >> the UEFI specification.
> > >>
> > >> Work around by using an alignment of at least 512 bytes in case
>
> When you look at the fix it is not obvious. So, I would update this
> sentence. Hmmm... I would prefer to drop 512 number from here
>
> > >> alignment is requested. Also make sure that IoAlign is still respected
> > >> as per UEFI specification if a higher alignment than block size is
> > >> requested.
>
> Could you explain how did you come up with this algorithm? Why did not
> you use 512 number directly as lower limit as Heinrich suggested? And
> your algorithm will not work for buggy UEFI implementations which has
> IoAlign > 9. Do we care?
Ugh... Sorry, I missed earlier thread. So, I am rewinding discussion a bit.
Anyway, please expand commit message and include as much information as
possible. Mostly it should come from the discussion around v1.
Daniel
> > >> Note: The problem has only noticed with compressed squashfs. It seems
> > >> that ext4 (and presumably other file system drivers) pass buffers with
> > >> a higher alignment already.
> > >>
> > >> Signed-off-by: Stefan Agner
> > >> ---
> > >> grub-core/disk/efi/efidisk.c | 18 --
> > >> 1 file changed, 16 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/grub-core/disk/efi/efidisk.c b/grub-core/disk/efi/efidisk.c
> > >> index 562543b35..dec4e2e8f 100644
> > >> --- a/grub-core/disk/efi/efidisk.c
> > >> +++ b/grub-core/disk/efi/efidisk.c
> > >> @@ -553,8 +553,22 @@ grub_efidisk_readwrite (struct grub_disk *disk,
> > >> grub_disk_addr_t sector,
> > >> d = disk->data;
> > >> bio = d->block_io;
> > >>
> > >> - /* Set alignment to 1 if 0 specified */
> > >> - io_align = bio->media->io_align ? bio->media->io_align : 1;
> > >> + /*
> > >> + * If IoAlign is > 1, it should represent the required alignment.
> > >> However,
> > >> + * some UEFI implementation on Intel NUC systems seem to use
> > >> IoAlign=2 but
> > >> + * require 2^IoAlign.
> > >> + * Make sure to align to at least block size if IO alignment is
> > >> required.
> > >> + */
> > >> + if (bio->media->io_align > 1)
> > >> +{
> > >> + if (bio->media->io_align < bio->media->block_size)
> > >
> > > block_size is not required to be a power of two (though it typically
> > > is). But io_align should be a power of two. So here some logic is
> > > missing. E.g.
> > >
> > > if (bio->media->io_align < bio->media->block_size &&
> > > !(bio->media->block_size & (bio->media->block_size - 1))
> > >
> > > Or simply use a fixed value 512 as lower limit.
> >
> > This code in grub_efidisk_open should take care of that:
> >
> > if (m->block_size & (m->block_size - 1) || !m->block_size)
> > return grub_error (GRUB_ERR_IO, "invalid sector size %d",
> >m->block_size);
>
> This should be mentioned in the commit message.
>
> Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2] efidisk: pass buffers with higher alignment
On 2022-05-17 17:21, Daniel Kiper wrote:
> On Tue, May 17, 2022 at 05:07:49PM +0200, Daniel Kiper wrote:
>> On Tue, May 10, 2022 at 10:40:15PM +0200, Stefan Agner wrote:
>> > On 2022-05-10 22:26, Heinrich Schuchardt wrote:
>> > > On 5/10/22 21:59, Stefan Agner wrote:
>> > >> Despite the UEFI specification saying "the requirement is that the
>> > >> start address of a buffer must be evenly divisible by IoAlign with
>> > >> no remainder.", it seems that a higher alignment requirement is
>> > >> neecssary on some system (e.g. a Intel NUC system with NVMe SSD).
>> > >> That particular system has IoAlign set to 2, and sometimes returns
>> > >> status 7 when buffers with alignment of 2 are passed. Things seem
>> > >> to work fine with buffers aligned to 4 bytes.
>> > >>
>> > >> It seems that some system interpret IoAlign > 1 to mean 2 ^ IoAlign.
>> > >> There is also such a hint in an example printed in the Driver Writer's
>> > >> Guide:
>> > >> ScsiPassThruMode.IoAlign = 2; // Data must be alligned on 4-byte
>> > >> boundary
>>
>> I think this confusion comes from improper interpretation of this part
>> of UEFI spec: IoAlign Supplies the alignment requirement for any buffer
>> used in a data transfer. IoAlign values of 0 and 1 mean that the buffer
>> can be placed anywhere in memory. Otherwise, IoAlign must be a power of
>> 2, and...
>>
>> I think it would be nice to point that out in the commit message.
>>
Agreed, this is likely due to misinterpretation of that section. Also,
the section stayed the same since the first revision I was able to
access (1.10).
I'll add that to the commit message.
>> > >> However, some systems (e.g. U-Boot and some drivers in EDK II) do follow
>> > >> the UEFI specification.
>> > >>
>> > >> Work around by using an alignment of at least 512 bytes in case
>>
>> When you look at the fix it is not obvious. So, I would update this
>> sentence. Hmmm... I would prefer to drop 512 number from here
>>
>> > >> alignment is requested. Also make sure that IoAlign is still respected
>> > >> as per UEFI specification if a higher alignment than block size is
>> > >> requested.
>>
>> Could you explain how did you come up with this algorithm? Why did not
>> you use 512 number directly as lower limit as Heinrich suggested? And
>> your algorithm will not work for buggy UEFI implementations which has
>> IoAlign > 9. Do we care?
>
> Ugh... Sorry, I missed earlier thread. So, I am rewinding discussion a bit.
> Anyway, please expand commit message and include as much information as
> possible. Mostly it should come from the discussion around v1.
I was just drafting an email pointing out that it should work since I
don't do the bit shifting stuff from v1 anymore :)
Will send a revision with updated commit message.
--
Stefan
>
> Daniel
>
>> > >> Note: The problem has only noticed with compressed squashfs. It seems
>> > >> that ext4 (and presumably other file system drivers) pass buffers with
>> > >> a higher alignment already.
>> > >>
>> > >> Signed-off-by: Stefan Agner
>> > >> ---
>> > >> grub-core/disk/efi/efidisk.c | 18 --
>> > >> 1 file changed, 16 insertions(+), 2 deletions(-)
>> > >>
>> > >> diff --git a/grub-core/disk/efi/efidisk.c b/grub-core/disk/efi/efidisk.c
>> > >> index 562543b35..dec4e2e8f 100644
>> > >> --- a/grub-core/disk/efi/efidisk.c
>> > >> +++ b/grub-core/disk/efi/efidisk.c
>> > >> @@ -553,8 +553,22 @@ grub_efidisk_readwrite (struct grub_disk *disk,
>> > >> grub_disk_addr_t sector,
>> > >> d = disk->data;
>> > >> bio = d->block_io;
>> > >>
>> > >> - /* Set alignment to 1 if 0 specified */
>> > >> - io_align = bio->media->io_align ? bio->media->io_align : 1;
>> > >> + /*
>> > >> + * If IoAlign is > 1, it should represent the required alignment.
>> > >> However,
>> > >> + * some UEFI implementation on Intel NUC systems seem to use
>> > >> IoAlign=2 but
>> > >> + * require 2^IoAlign.
>> > >> + * Make sure to align to at least block size if IO alignment is
>> > >> required.
>> > >> + */
>> > >> + if (bio->media->io_align > 1)
>> > >> +{
>> > >> + if (bio->media->io_align < bio->media->block_size)
>> > >
>> > > block_size is not required to be a power of two (though it typically
>> > > is). But io_align should be a power of two. So here some logic is
>> > > missing. E.g.
>> > >
>> > > if (bio->media->io_align < bio->media->block_size &&
>> > > !(bio->media->block_size & (bio->media->block_size - 1))
>> > >
>> > > Or simply use a fixed value 512 as lower limit.
>> >
>> > This code in grub_efidisk_open should take care of that:
>> >
>> > if (m->block_size & (m->block_size - 1) || !m->block_size)
>> > return grub_error (GRUB_ERR_IO, "invalid sector size %d",
>> > m->block_size);
>>
>> This should be mentioned in the commit message.
>>
>> Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3] util/grub.d/linux: Improve initramfs detection
On Mon, May 02, 2022 at 11:12:56PM -0500, Oskari Pirhonen wrote: > Add detection for initramfs of the form *.img.old. For example, Gentoo's > sys-kernel/genkernel installs it as initramfs-*.img and moves any > existing one to initramfs-*.img.old. > > Apply the same scheme to initrd-*.img and initrd-*.gz files for > consistency. > > Signed-off-by: Oskari Pirhonen Reviewed-by: Daniel Kiper Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
