Re: grub-mkstandalone Error
On Mon, Sep 21, 2020 at 10:53 AM Wasim Khan wrote: > > Hi, > > I am facing below error while generating standalone image if I use Linaro GCC > 7.4-2019.02 toolchain. > > $ grub-mkstandalone --directory=./grub-core -O arm64-efi -o grub.efi > --modules "tftp net efinet gzio linux efifwsetup part_gpt part_msdos font > gfxterm all_video" /boot/grub/grub.cfg=./grub.cfg > grub-mkstandalone: error: relocation 0x113 is not implemented yet. This is relocation R_AARCH64_ADR_PREL_PG_HI21 The weird part is that I implemented it back in 2016. Can you retest with official source? > > Above command works fine if I use linaro-5.4.1 toolchain. > Can somebody please help to know why the issues is coming with newer > toolchain (like GCC 7.4-2019.02) and any fix for that ? > > > Regards, > Wasim > -- Regards Vladimir 'phcoder' Serbinenko ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3 9/9] cryptodisk: Properly handle non-512 byte sized sectors
On Mon, Sep 21, 2020 at 05:58:06AM +, Glenn Washburn wrote:
> Sep 9, 2020 5:22:11 AM Daniel Kiper :
>
> > On Mon, Sep 07, 2020 at 05:28:08PM +0200, Patrick Steinhardt wrote:
> >> From: Glenn Washburn
> >>
> >> 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 cipher block in the fifth
> >> 4K sector will be encrypted with an IV equal to 32, as opposed to 32-39 for
> >
> > s/32-39/32-9/?
>
> I'm reading this and realizing it's worded badly and confusing. Perhaps this
> sentence is better?
>
> "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."
Yeah, LGTM.
> >> each sequential 512 byte block or an IV of 4 for each cipher block 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
> >> Reviewed-by: Patrick Steinhardt
> >> ---
> >> grub-core/disk/cryptodisk.c | 44 -
> >> grub-core/disk/luks.c | 5 +++--
> >> grub-core/disk/luks2.c | 7 +-
> >> include/grub/cryptodisk.h | 9 +++-
> >> 4 files changed, 41 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> >> index 0b63b7d96..6319f3164 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,12 +238,13 @@ 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)
> >> / sizeof (grub_uint32_t));
> >> grub_uint32_t iv[(GRUB_CRYPTO_MAX_CIPHER_BLOCKSIZE + 3) / 4];
> >> + grub_uint64_t iv_calc = 0;
> >>
> >> if (dev->rekey)
> >> {
> >> @@ -270,7 +272,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 +283,16 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk
> >> *dev,
> >> }
> >> break;
> >> case GRUB_CRYPTODISK_MODE_IV_PLAIN64:
> >> - iv[1] = grub_cpu_to_le32 (sector >> 32);
> >> + iv_calc = sector << (log_sector_size - GRUB_CRYPTODISK_IV_LOG_SIZE);
> >> + iv[1] = grub_cpu_to_le32 (iv_calc >> 32);
> >
> > Why 32? Could you use a constant or add a comment here?
>
> Plain mode uses a 32 bit IV and plain64 uses a 64 bit IV. So in the
> plain64 case only deal with the non-plain (ie not lower 32 bits) IV
> bits and we deal with the plain case in the fall through.
>
> I don't think a constant is warranted and I can add in a comment in
> this patch, but I'd like to point out that the "32" literal you're
> commenting on is not code I've introduced. As such, the comment would
> not be relevant to this patch. Given that, do you still want a comment
> in this patch?
If you tou
Re: [PATCH v3 4/9] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors
On Mon, Sep 21, 2020 at 06:28:28AM +, Glenn Washburn wrote: > Sep 8, 2020 7:21:31 AM Daniel Kiper : > > On Mon, Sep 07, 2020 at 05:27:46PM +0200, Patrick Steinhardt wrote: > >> From: Glenn Washburn > >> > >> The total_length field is named confusingly because length usually refers > >> to > >> bytes, whereas in this case its really the total number of sectors on the > >> device. Also counter-intuitively, grub_disk_get_size returns the total > > > > Could we change total_length name? Or should it stay as is because this > > name is used in other implementations too? > > I sent a patch which renamed total_length to total_sectors. I believe > Patrick chose not to include it because I did not fix a bug in the > code and this patch series was only patches he thought essential to be > included in the next release. I'll include that patch again in a > follow up patch series. Please do. I want to have this fixed before 2.06 release... > >> number of device native sectors sectors. We need to convert the sectors > >> from > >> the size of the underlying device to the cryptodisk sector size. And > >> segment.size is in bytes which need to be converted to cryptodisk sectors. > >> > >> Signed-off-by: Glenn Washburn > >> Reviewed-by: Patrick Steinhardt > >> --- > >> grub-core/disk/luks2.c | 7 --- > >> 1 file changed, 4 insertions(+), 3 deletions(-) > >> > >> diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c > >> index c4c6ac90c..5f15a4d2c 100644 > >> --- a/grub-core/disk/luks2.c > >> +++ b/grub-core/disk/luks2.c > >> @@ -417,7 +417,7 @@ luks2_decrypt_key (grub_uint8_t *out_key, > >> grub_uint8_t salt[GRUB_CRYPTODISK_MAX_KEYLEN]; > >> grub_uint8_t *split_key = NULL; > >> grub_size_t saltlen = sizeof (salt); > >> - char cipher[32], *p;; > >> + char cipher[32], *p; > > > > I am OK with changes like that but they should be mentioned shortly in > > the commit message. > > Noted, I'll put update the commit message. > > >> const gcry_md_spec_t *hash; > >> gcry_err_code_t gcry_ret; > >> grub_err_t ret; > >> @@ -603,9 +603,10 @@ luks2_recover_key (grub_disk_t disk, > >> crypt->log_sector_size = sizeof (unsigned int) * 8 > >> - __builtin_clz ((unsigned int) segment.sector_size) - 1; > >> if (grub_strcmp (segment.size, "dynamic") == 0) > >> - crypt->total_length = grub_disk_get_size (disk) - crypt->offset; > >> + crypt->total_length = (grub_disk_get_size (disk) >> > >> (crypt->log_sector_size - disk->log_sector_size)) > >> + - crypt->offset; > >> else > >> - crypt->total_length = grub_strtoull (segment.size, NULL, 10); > >> + crypt->total_length = grub_strtoull (segment.size, NULL, 10) >> > >> crypt->log_sector_size; > > > > I do not like that you ignore grub_strtoull() errors. Additionally, what > > will happen if segment.size is smaller than LUKS2 sector size? Should > > not you round segment.size up to the nearest multiple of LUKS2 sector > > size first? I think the same applies to the earlier change too. > > Again, I was making a minimal set of changes for this fix. Your > comments about grub_strtoull, while valid, don't apply to this patch > and should be addressed in a new patch. OK, please fix it then in separate patch. > Your concern about rounding segment.size up, is also valid and > pertinent to this patch, I'll update that in a following patch series. > This may get more complicated if the last partial sector is at the end > of the disk. Yeah, but please try to fix it somehow... Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3 3/9] luks2: Fix use of incorrect index and some error messages
On Mon, Sep 21, 2020 at 06:45:04AM +, Glenn Washburn wrote: > Sep 8, 2020 6:58:48 AM Daniel Kiper : > > On Mon, Sep 07, 2020 at 05:27:41PM +0200, Patrick Steinhardt wrote: > >> From: Glenn Washburn > > > > It seems to me this patch should be split into two and and begs for > > commit message improvement. In general it would be nice to know why > > we need these fixes. > > This patch is doing two things, fixing a bug and making the code more > understandable. My original patch just fixed the bug, then Patrick > suggested making it more readable. I can split it into two patches. That will be perfect! Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3 6/9] cryptodisk: Unregister cryptomount command when removing module
On Mon, Sep 21, 2020 at 06:45:57AM +, Glenn Washburn wrote:
> Sep 8, 2020 7:28:13 AM Daniel Kiper :
> > On Mon, Sep 07, 2020 at 05:27:55PM +0200, Patrick Steinhardt wrote:
> >> From: Glenn Washburn
> >>
> >> Signed-off-by: Glenn Washburn
> >> Reviewed-by: Patrick Steinhardt
> >> ---
> >> grub-core/disk/cryptodisk.c | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> >> index 1897acc4b..b2c6e9a7d 100644
> >> --- a/grub-core/disk/cryptodisk.c
> >> +++ b/grub-core/disk/cryptodisk.c
> >> @@ -1311,5 +1311,6 @@ GRUB_MOD_FINI (cryptodisk)
> >> {
> >> grub_disk_dev_unregister (&grub_cryptodisk_dev);
> >> cryptodisk_cleanup ();
> >
> > I am OK with this patch but I realized that cryptodisk_cleanup() body is
> > commented out, So, could you add another patch which drops
> > cryptodisk_cleanup() entirely from the GRUB code? Or make it work as it
> > supposed to work. Maybe the latter is better...
> >
> > Daniel
> >
>
> I can look in to this when I get to the other changes.
OK...
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
grub-mkstandalone Error
Hi, I am facing below error while generating standalone image if I use Linaro GCC 7.4-2019.02 toolchain. $ grub-mkstandalone --directory=./grub-core -O arm64-efi -o grub.efi --modules "tftp net efinet gzio linux efifwsetup part_gpt part_msdos font gfxterm all_video" /boot/grub/grub.cfg=./grub.cfg grub-mkstandalone: error: relocation 0x113 is not implemented yet. Above command works fine if I use linaro-5.4.1 toolchain. Can somebody please help to know why the issues is coming with newer toolchain (like GCC 7.4-2019.02) and any fix for that ? Regards, Wasim ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: Multiboot2 on aarch64: Alignment of ELF Headers
On Sat, 2020-05-23 at 14:33 +0200, Hans Ulrich Niedermann wrote:
> On Sat, 23 May 2020 12:21:27 +0100
> Chris Plant via Grub-devel wrote:
>
> > On Sat, 2020-05-23 at 12:43 +0200, Hans Ulrich Niedermann wrote:
> > > On Fri, 22 May 2020 17:23:35 +0100
> > > Chris Plant via Grub-devel wrote:
> > >
> > > > I'm continuing to work on Multiboot2 support on aarch64, and
> > > > I'm
> > > > looking at the alignment of the ELF headers which are passed
> > > > through
> > > > MB2.
> > >
> > > At the risk of me sounding stupid...
> > >
> > > Having read the MB2 specs quite thoroughly in the past few
> > > months, I
> > > still have no idea what "ELF headers" being "passed through MB2"
> > > could
> > > be about. The MB2 spec defines a MB2 header consisting of a four
> > > MB2
> > > header magic fields and a set of MB2 header tags.
> > >
> > > The only thing ELF related I could find there is the MB2 header
> > > address
> > > tag which duplicates some information from ELF (if OS image is in
> > > ELF format) or contains address information (if OS image is in
> > > non-ELF format). Is that what you are talking about?
> >
> > Yes, this is what I'm talking about. I'm using the MB2 tag (type
> > 9)
> > to see where the ELF section headers are.
>
> Ah. So the type 9 you are referring to is the "ELF-Symbols" boot
> information tag (i.e. information the bootloader communicates to the
> OS
> image), not an MB2 header tag (information the OS image communicates
> to
> the bootloader).
>
> I am catching up with you, finally.
>
> > Apologies, I've not been clear enough and rambled too much. The
> > issue
> > I see is nothing to do with grub's MB2 implementation, but instead
> > in
> > the way grub is loading the ELF section headers.
> >
> > The ELF section headers appear to be loaded on a 4 byte alignment
> > not
> > an 8 byte alignment. You can read the 4 byte members of the ELF
> > header structure fine, but not the 8 byte members as they are
> > aligned
> > to 4 bytes.
> >
> > So, my question is, should grub load the ELF headers to an 8 byte
> > alignment, or should the OS be prepared for them to be aligned to 4
> > byte?
>
> The 64bit part of the ELF spec appears to specifically made such that
> 64bit values are aligned to 64bit boundaries (I have taken a look at
> https://en.wikipedia.org/wiki/Executable_and_Linkable_Format for an
> overview).
>
> Therefore, loading an ELF64 image with its many 64bit values to an
> address not aligned to 8 byte boundaries sounds to me like a probable
> oversight going back to ELF32 days and thus a bug for ELF64, unless
> someone has a very good argument to the contrary.
On this basis, is the logical change to fix the definition of the "ELF-
Symbols" boot information tag, which is currently:
struct multiboot_tag_elf_sections
{
multiboot_uint32_t type;
multiboot_uint32_t size;
multiboot_uint32_t num;
multiboot_uint32_t entsize;
multiboot_uint32_t shndx;
char sections[0];
};
grub aligns this to an 8 byte boundary, but then the odd number of
uint32_t mean that sections is on a 4 byte boundary and is designed
within it's ELF defintion to be on 8 byte boundary.
So I would propose adding a 4 byte pad between shndx and sections, so
that sections on an 8 byte boundary. This might break things until
they're recompiled but won't require any changes to code elsewhere and
I can't see how else to fix it without a new information defintion.
I can include this in my patch (first one, taking time and will be
terrible) to enable multiboot2 on ARM64 if this is the approach
required/desired?
Chris
>
> Uli
>
> ___
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 2/4] travis: Fix sparc64 build
Hi Daniel,
thanks for merging first commit.
> On Wed, Sep 09, 2020 at 10:02:19PM +0200, Petr Vorel wrote:
> > Instead non-existing sparc64-ieee1275 format use all 3 sparc64 formats:
> > sparc64-ieee1275-{aout,cdcore,raw}.
> > Signed-off-by: Petr Vorel
> > ---
> > .travis.yml | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> > diff --git a/.travis.yml b/.travis.yml
> > index 4bd05a30a..d019a0172 100644
> > --- a/.travis.yml
> > +++ b/.travis.yml
> > @@ -40,8 +40,8 @@ script:
> ># Build all selected GRUB targets.
> >- for target in $GRUB_TARGETS; do
> > - plat=${target#*-};
> > - arch=${target%-*};
> > + arch=$(echo $target | cut -d- -f1);
> > + plat=$(echo $target | cut -d- -f2);
> I think it should be: plat=$(echo $target | cut -d- -f2-);
> Note dash after "2"...
No, this way it breaks sparc64 [1] (and does not help to fix mips [2]
nor risc32 either).
It looks like it's this error:
configure: error: platform "ieee1275-raw" is not supported for target CPU
"sparc64"
but I need to debug it more to see what's wrong.
Kind regards,
Petr
> Though I would prefer this:
> arch=$(echo "$target" | cut -d - -f 1);
> plat=$(echo "$target" | cut -d - -f 2-);
> Daniel
Kind regards,
Petr
[1] https://travis-ci.org/github/pevik/grub/jobs/728077121
[2] https://travis-ci.org/github/pevik/grub/jobs/728077123
[3] https://travis-ci.org/github/pevik/grub/jobs/728077126
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: Multiboot2 on aarch64: Alignment of ELF Headers
On 21.09.2020 21:14, Chris Plant wrote:
On Sat, 2020-05-23 at 14:33 +0200, Hans Ulrich Niedermann wrote:
On Sat, 23 May 2020 12:21:27 +0100
Chris Plant via Grub-devel wrote:
On Sat, 2020-05-23 at 12:43 +0200, Hans Ulrich Niedermann wrote:
On Fri, 22 May 2020 17:23:35 +0100
Chris Plant via Grub-devel wrote:
I'm continuing to work on Multiboot2 support on aarch64, and
I'm
looking at the alignment of the ELF headers which are passed
through
MB2.
At the risk of me sounding stupid...
Having read the MB2 specs quite thoroughly in the past few
months, I
still have no idea what "ELF headers" being "passed through MB2"
could
be about. The MB2 spec defines a MB2 header consisting of a four
MB2
header magic fields and a set of MB2 header tags.
The only thing ELF related I could find there is the MB2 header
address
tag which duplicates some information from ELF (if OS image is in
ELF format) or contains address information (if OS image is in
non-ELF format). Is that what you are talking about?
Yes, this is what I'm talking about. I'm using the MB2 tag (type
9)
to see where the ELF section headers are.
Ah. So the type 9 you are referring to is the "ELF-Symbols" boot
information tag (i.e. information the bootloader communicates to the
OS
image), not an MB2 header tag (information the OS image communicates
to
the bootloader).
I am catching up with you, finally.
Apologies, I've not been clear enough and rambled too much. The
issue
I see is nothing to do with grub's MB2 implementation, but instead
in
the way grub is loading the ELF section headers.
The ELF section headers appear to be loaded on a 4 byte alignment
not
an 8 byte alignment. You can read the 4 byte members of the ELF
header structure fine, but not the 8 byte members as they are
aligned
to 4 bytes.
So, my question is, should grub load the ELF headers to an 8 byte
alignment, or should the OS be prepared for them to be aligned to 4
byte?
The 64bit part of the ELF spec appears to specifically made such that
64bit values are aligned to 64bit boundaries (I have taken a look at
https://en.wikipedia.org/wiki/Executable_and_Linkable_Format for an
overview).
Therefore, loading an ELF64 image with its many 64bit values to an
address not aligned to 8 byte boundaries sounds to me like a probable
oversight going back to ELF32 days and thus a bug for ELF64, unless
someone has a very good argument to the contrary.
On this basis, is the logical change to fix the definition of the "ELF-
Symbols" boot information tag, which is currently:
struct multiboot_tag_elf_sections
{
multiboot_uint32_t type;
multiboot_uint32_t size;
multiboot_uint32_t num;
multiboot_uint32_t entsize;
multiboot_uint32_t shndx;
char sections[0];
};
grub aligns this to an 8 byte boundary, but then the odd number of
uint32_t mean that sections is on a 4 byte boundary and is designed
within it's ELF defintion to be on 8 byte boundary.
So I would propose adding a 4 byte pad between shndx and sections, so
that sections on an 8 byte boundary. This might break things until
they're recompiled but won't require any changes to code elsewhere and
I can't see how else to fix it without a new information defintion.
This WILL break things until they are recompiled in every MB2 kernel,
in addition to grub, every other MB2-capable bootloader and documentation.
Until then, the kernel would have to somewhat guess whether it was given
the old (not aligned) tag or the new one. While it may be possible as long
as the padding is filled with a value that cannot appear in the first bytes
of sections, the users would have to add the discovery code into kernels.
MB2 is one of the most commonly used (if not the most common) boot protocols
for hobby OSes.
IMHO the only way out of it would be to create a new tag type and leave the
old one as it is right now. This won't break current implementations, though
it will pass redundant information. After some transition period we may drop
the old format.
I can include this in my patch (first one, taking time and will be
terrible) to enable multiboot2 on ARM64 if this is the approach
required/desired?
Chris
Uli
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
--
Krystian Hebel
Firmware Engineer
https://3mdeb.com | @3mdeb_com
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: Multiboot2 on aarch64: Alignment of ELF Headers
On Tue, 2020-09-22 at 08:33 +0200, Krystian Hebel wrote:
> On 21.09.2020 21:14, Chris Plant wrote:
> > On Sat, 2020-05-23 at 14:33 +0200, Hans Ulrich Niedermann wrote:
> > > On Sat, 23 May 2020 12:21:27 +0100
> > > Chris Plant via Grub-devel wrote:
> > >
> > > > On Sat, 2020-05-23 at 12:43 +0200, Hans Ulrich Niedermann
> > > > wrote:
> > > > > On Fri, 22 May 2020 17:23:35 +0100
> > > > > Chris Plant via Grub-devel wrote:
> > > > >
> > > > > > I'm continuing to work on Multiboot2 support on aarch64,
> > > > > > and
> > > > > > I'm
> > > > > > looking at the alignment of the ELF headers which are
> > > > > > passed
> > > > > > through
> > > > > > MB2.
> > > > > At the risk of me sounding stupid...
> > > > >
> > > > > Having read the MB2 specs quite thoroughly in the past few
> > > > > months, I
> > > > > still have no idea what "ELF headers" being "passed through
> > > > > MB2"
> > > > > could
> > > > > be about. The MB2 spec defines a MB2 header consisting of a
> > > > > four
> > > > > MB2
> > > > > header magic fields and a set of MB2 header tags.
> > > > >
> > > > > The only thing ELF related I could find there is the MB2
> > > > > header
> > > > > address
> > > > > tag which duplicates some information from ELF (if OS image
> > > > > is in
> > > > > ELF format) or contains address information (if OS image is
> > > > > in
> > > > > non-ELF format). Is that what you are talking about?
> > > > Yes, this is what I'm talking about. I'm using the MB2 tag
> > > > (type
> > > > 9)
> > > > to see where the ELF section headers are.
> > > Ah. So the type 9 you are referring to is the "ELF-Symbols" boot
> > > information tag (i.e. information the bootloader communicates to
> > > the
> > > OS
> > > image), not an MB2 header tag (information the OS image
> > > communicates
> > > to
> > > the bootloader).
> > >
> > > I am catching up with you, finally.
> > >
> > > > Apologies, I've not been clear enough and rambled too
> > > > much. The
> > > > issue
> > > > I see is nothing to do with grub's MB2 implementation, but
> > > > instead
> > > > in
> > > > the way grub is loading the ELF section headers.
> > > >
> > > > The ELF section headers appear to be loaded on a 4 byte
> > > > alignment
> > > > not
> > > > an 8 byte alignment. You can read the 4 byte members of the
> > > > ELF
> > > > header structure fine, but not the 8 byte members as they are
> > > > aligned
> > > > to 4 bytes.
> > > >
> > > > So, my question is, should grub load the ELF headers to an 8
> > > > byte
> > > > alignment, or should the OS be prepared for them to be aligned
> > > > to 4
> > > > byte?
> > > The 64bit part of the ELF spec appears to specifically made such
> > > that
> > > 64bit values are aligned to 64bit boundaries (I have taken a look
> > > at
> > > https://en.wikipedia.org/wiki/Executable_and_Linkable_Format for
> > > an
> > > overview).
> > >
> > > Therefore, loading an ELF64 image with its many 64bit values to
> > > an
> > > address not aligned to 8 byte boundaries sounds to me like a
> > > probable
> > > oversight going back to ELF32 days and thus a bug for ELF64,
> > > unless
> > > someone has a very good argument to the contrary.
> > On this basis, is the logical change to fix the definition of the
> > "ELF-
> > Symbols" boot information tag, which is currently:
> >
> > struct multiboot_tag_elf_sections
> > {
> >multiboot_uint32_t type;
> >multiboot_uint32_t size;
> >multiboot_uint32_t num;
> >multiboot_uint32_t entsize;
> >multiboot_uint32_t shndx;
> >char sections[0];
> > };
> >
> > grub aligns this to an 8 byte boundary, but then the odd number of
> > uint32_t mean that sections is on a 4 byte boundary and is designed
> > within it's ELF defintion to be on 8 byte boundary.
> >
> > So I would propose adding a 4 byte pad between shndx and sections,
> > so
> > that sections on an 8 byte boundary. This might break things until
> > they're recompiled but won't require any changes to code elsewhere
> > and
> > I can't see how else to fix it without a new information defintion.
> This WILL break things until they are recompiled in every MB2 kernel,
> in addition to grub, every other MB2-capable bootloader and
> documentation.
> Until then, the kernel would have to somewhat guess whether it was
> given
> the old (not aligned) tag or the new one. While it may be possible as
> long
> as the padding is filled with a value that cannot appear in the first
> bytes
> of sections, the users would have to add the discovery code into
> kernels.
> MB2 is one of the most commonly used (if not the most common) boot
> protocols
> for hobby OSes.
Understood.
>
> IMHO the only way out of it would be to create a new tag type and
> leave the
> old one as it is right now. This won't break current implementations,
> though
> it will pass redundant information. After some transition period we
> may drop
> the old format.
I'll create a new tag, struct multiboot_tag_elf64_sections which is
correctly aligned.
