Re: [PATCH] Drop gnulib fix-base64.patch
Robbie Harwood writes:
> Originally added in 9fbdec2f6b4fa8b549daa4d49134d1fe89d95ef9 and
> subsequently modified in 552c9fd08122a3036c724ce96dfe68aa2f75705f,
> fix-base64.patch handled two problems we have using gnulib, which are
> exerciesd by the base64 module but not directly caused by it.
>
> First, grub2 defines its own bool type, while gnulib expects the
> equivalent of stdbool.h to be present. Rather than patching gnulib,
> instead use gnulib's stdbool module to provide a bool type if needed.
> (Suggested by Simon Josefsson.)
>
> Second, our config.h doesn't always inherit config-util.h, which is
> where gnulib-related options like _GL_ATTRIBUTE_CONST end up.
> fix-base64.h worked around this by defining the attribute away, but this
> workaround is better placed in config.h itself, not a gnulib patch.
>
> Signed-off-by: Robbie Harwood
> ---
> bootstrap.conf| 3 ++-
> config.h.in | 3 +++
> grub-core/lib/gnulib-patches/fix-base64.patch | 21 ---
> grub-core/lib/posix_wrap/sys/types.h | 7 +++
> grub-core/lib/xzembed/xz.h| 5 +
> 5 files changed, 9 insertions(+), 30 deletions(-)
> delete mode 100644 grub-core/lib/gnulib-patches/fix-base64.patch
>
> diff --git a/bootstrap.conf b/bootstrap.conf
> index 0dd893c5c..21a8cf15d 100644
> --- a/bootstrap.conf
> +++ b/bootstrap.conf
> @@ -35,6 +35,7 @@ gnulib_modules="
>realloc-gnu
>regex
>save-cwd
> + stdbool
> "
>
> gnulib_tool_option_extras="\
> @@ -80,7 +81,7 @@ cp -a INSTALL INSTALL.grub
>
> bootstrap_post_import_hook () {
>set -e
> - for patchname in fix-base64 fix-null-deref fix-null-state-deref
> fix-regcomp-uninit-token \
> + for patchname in fix-null-deref fix-null-state-deref
> fix-regcomp-uninit-token \
>fix-regexec-null-deref fix-uninit-structure fix-unused-value fix-width
> no-abort; do
> patch -d grub-core/lib/gnulib -p2 \
>< "grub-core/lib/gnulib-patches/$patchname.patch"
> diff --git a/config.h.in b/config.h.in
> index 9e8f9911b..2b65c86c4 100644
> --- a/config.h.in
> +++ b/config.h.in
> @@ -64,4 +64,7 @@
>
> #define _GNU_SOURCE 1
>
> +/* For gnulib's base64 code. */
> +#define _GL_ATTRIBUTE_CONST /* empty */
Do we support any compiler so old or configuration so weird that we
can't simply use 'const' here?
Kind regards,
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 0/2] powerpc-ieee1275: support larger core.elf images
Hello Daniel! On 11/16/21 04:42, Daniel Axtens wrote: > Daniel Axtens (2): > powerpc-ieee1275: load grub at 8MB, not 2MB > ieee1275: set real-base in the PowerPC IEEE1275 Note to 32MB I have applied these now and tested them on my iBook G4 but that introduced the following regression: root@ibook-g4-14:/home/glaubitz/grub# grub-install --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. grub-install: error: `/usr/local/lib/grub/powerpc-ieee1275/kernel.img' is miscompiled: its start address is 0x20 instead of 0x80: ld.gold bug?. root@ibook-g4-14:/home/glaubitz/grub# Adrian -- .''`. John Paul Adrian Glaubitz : :' : Debian Developer - glaub...@debian.org `. `' Freie Universitaet Berlin - glaub...@physik.fu-berlin.de `-GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Drop gnulib fix-base64.patch
Daniel Axtens writes: > Robbie Harwood writes: > >> +/* For gnulib's base64 code. */ >> +#define _GL_ATTRIBUTE_CONST /* empty */ > > Do we support any compiler so old or configuration so weird that we > can't simply use 'const' here? Unfortunately it's not quite that simple. _GL_ATTRIBUTE_CONST actually gates turning on `__attribute ((__const__))`, which if memory serves is a GCC extension. I don't know what the support matrix on grub is, but based on the original patch there was a need to support non-gcc-likes. Would be fine with changing that, though. (Personally, I think it should be obvious to a reasonable compiler that isbase64() is purely arithmetic. However, gnulib upstream did not like my proposal to drop the attribute marker, so we're stuck with it.) Be well, --Robbie signature.asc Description: PGP signature ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [RESEND v3 0/3] use confidential computing provisioned secrets for disk decryption
On Thu, Nov 18, 2021 at 12:15:55PM -0500, James Bottomley wrote: > On Thu, 2021-11-18 at 15:49 +0100, Daniel Kiper wrote: > > Hey, > > > > Adding Denis, Patrick and Glenn... > > > > James, please add them to the loop next time. > > Sure ... is there some way of telling who should be cc'd (I'm not a fan > of the kernel get_maintainer.pl but it gives you a list you can trim)? You can take a look at the MAINTAINERS files. Sadly it only lists core maintainers who should be CC-ed. If you CC them they will tell you if you should add anybody else. I know it is not perfect but... I hope we will improve this at some point. > > On Tue, Nov 09, 2021 at 08:53:53AM -0500, James Bottomley wrote: > > > From: James Bottomley > > > > > > v3: make password getter specify prompt requirement. Update for > > > TDX: > > > Make name more generic and expand size of secret area > > > > > > > > > https://github.com/tianocore/edk2/commit/96201ae7bf97c3a2c0ef386110bb93d25e9af1ba > > > > > > https://github.com/tianocore/edk2/commit/caf8b3872ae2ac961c9fdf4d1d2c5d072c207299 > > > > > > Redo the cryptodisk secret handler to make it completely > > > generic > > > and pluggable using a list of named secret providers. Also > > > allow an optional additional argument for secret providers that may > > > have more than one secret. > > > > > > v2: update geli.c to use conditional prompt and add callback for > > > variable message printing and secret destruction > > > > > > To achieve encrypted disk images in the AMD SEV and other > > > confidential computing encrypted virtual machines, we need to add > > > the ability for grub to retrieve the disk passphrase from an OVMF > > > provisioned > > > configuration table. > > > > > > https://github.com/tianocore/edk2/commit/01726b6d23d4c8a870dbd5b96c0b9e3caf38ef3c > > > > > > The patches in this series modify grub to look for the disk > > > passphrase in the secret configuration table and use it to decrypt > > > any disks in the system if they are found. This is so an encrypted > > > image with a properly injected password will boot without any user > > > intervention. > > > > > > The three patches firstly modify the cryptodisk consumers to allow > > > arbitrary password getters instead of the current console based > > > one. The next patch adds a '-s module [id]' option to cryptodisk > > > to allow it to use plugin provided passwords and the final one adds > > > a sevsecret command to check for the secrets configuration table > > > and provision the disk passphrase from it if an entry is > > > found. With all this in place, the sequence to boot an encrypted > > > volume without user intervention is: > > > > > > cryptomount -s efisecret > > > source (crypto0)/boot/grub.cfg > > > > > > Assuming there's a standard Linux root partition. > > > > Thank you for posting this patch series. Unfortunately it conflicts > > with [1] patches. And I want to take [1] first because it is an > > important improvement for GRUB's crypto infrastructure. Additionally, > > as Glenn said in [1], this crypto infra change should simplify your > > code too. > > > > I have just finished reviewing Glenn's patches and waiting for v4. > > I hope we will be able to merge it soon. If you could take a look at > > the [1] and check if it does not make any troubles for you it would > > be perfect. > > > > I will drop you a line when Glenn's patches are in the tree and you > > can rebase your patch set on top of it. > > Yes, the rebase looks trivial. I'll do it and repost as soon as the > patches are upstream. Cool! Thanks! Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Drop gnulib fix-base64.patch
CC-ing Daren, Patrick and Vladimir... On Tue, Nov 23, 2021 at 11:08:55AM -0500, Robbie Harwood wrote: > Daniel Axtens writes: > > > Robbie Harwood writes: > > > >> +/* For gnulib's base64 code. */ > >> +#define _GL_ATTRIBUTE_CONST /* empty */ > > > > Do we support any compiler so old or configuration so weird that we > > can't simply use 'const' here? > > Unfortunately it's not quite that simple. _GL_ATTRIBUTE_CONST actually > gates turning on `__attribute ((__const__))`, which if memory serves is > a GCC extension. I don't know what the support matrix on grub is, but > based on the original patch there was a need to support non-gcc-likes. > Would be fine with changing that, though. > > (Personally, I think it should be obvious to a reasonable compiler that > isbase64() is purely arithmetic. However, gnulib upstream did not like > my proposal to drop the attribute marker, so we're stuck with it.) When I started looking at this issue I realized we have bigger problem here than lack of _GL_ATTRIBUTE_CONST definition. In general all _GL_* constants land in config-util.h.in and finally in config-util.h. It does not make a lot of sense because config-util.h is not included when you build GRUB core. Instead it is included when you build GRUB utils. Huh! AFAICT this means the gnulib is deprived of all its specific constants. I am not sure how it worked at all in so far. Anyway, I think we have to fix it properly. Robbie, may I ask you to talk to gnulib folks and ask them how we should solve this problem? Please CC all folks CC-ed in this email. Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 1/2] efinet: correct closing of SNP protocol
First of all, sorry for late reply but I am busy...
On Wed, Oct 06, 2021 at 10:30:42AM +0200, Heinrich Schuchardt wrote:
> In the context of the implementation of the EFI_LOAD_FILE2_PROTOCOL for
> the initial ramdisk it was observed that opening the SNP protocol failed.
> https://lists.gnu.org/archive/html/grub-devel/2021-10/msg00020.html
> This is due to an incorrect call to CloseProtocol().
>
> The first parameter of CloseProtocol() is the handle, not the interface.
>
> We call OpenProtocol() with ControllerHandle = NULL. Hence we must also
> call CloseProtcol with ControllerHandel = NULL.
>
> Each call of OpenProtocol() for the same network card handle is expected to
> return the same interface pointer. If we want to close the protocol which
> we opened non-exclusively when searching for a card, we have to do this
> before opening the protocol exclusively.
>
> As there is no guarantee that we successfully open the protocol add checks
> in the transmit and receive functions.
>
> Reported-by: Andreas Schwab
> Signed-off-by: Heinrich Schuchardt
> ---
> grub-core/net/drivers/efi/efinet.c | 32 ++
> 1 file changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/grub-core/net/drivers/efi/efinet.c
> b/grub-core/net/drivers/efi/efinet.c
> index 5388f952b..3f2ff03f5 100644
> --- a/grub-core/net/drivers/efi/efinet.c
> +++ b/grub-core/net/drivers/efi/efinet.c
> @@ -39,6 +39,8 @@ send_card_buffer (struct grub_net_card *dev,
>grub_uint64_t limit_time = grub_get_time_ms () + 4000;
>void *txbuf;
>
> + if (!net)
if (net == NULL)
> +return grub_error (GRUB_ERR_IO, N_("couldn't send network packet"));
Could you display more specific error here?
>if (dev->txbusy)
> while (1)
>{
> @@ -101,6 +103,9 @@ get_card_packet (struct grub_net_card *dev)
>struct grub_net_buff *nb;
>int i;
>
> + if (!net)
if (net == NULL)
> +return NULL;
> +
>for (i = 0; i < 2; i++)
> {
>if (!dev->rcvbuf)
> @@ -148,11 +153,23 @@ open_card (struct grub_net_card *dev)
> {
>grub_efi_simple_network_t *net;
>
> - /* Try to reopen SNP exlusively to close any active MNP protocol instance
> - that may compete for packet polling
> + if (dev->efi_net)
if (dev->efi_net != NULL)
... and please fix this in the other places in the patch(es) too...
> +{
> + efi_call_4 (grub_efi_system_table->boot_services->close_protocol,
> + dev->efi_handle, &net_io_guid,
> + grub_efi_image_handle, 0);
s/0/NULL/
> + dev->efi_net = NULL;
> +}
> + /*
> + * Try to reopen SNP exlusively to close any active MNP protocol instance
> + * that may compete for packet polling
> */
>net = grub_efi_open_protocol (dev->efi_handle, &net_io_guid,
> GRUB_EFI_OPEN_PROTOCOL_BY_EXCLUSIVE);
> + /* If exclusively fails, try non-exclusively. */
> + if (!net)
> + net = grub_efi_open_protocol (dev->efi_handle, &net_io_guid,
> + GRUB_EFI_OPEN_PROTOCOL_GET_PROTOCOL);
I am not convinced it is correct. Please take a look at send_card_buffer()...
46 st = efi_call_3 (net->get_status, net, 0, &txbuf);
47 if (st != GRUB_EFI_SUCCESS)
48 return grub_error (GRUB_ERR_IO,
49 N_("couldn't send network packet"));
50 /*
51Some buggy firmware could return an arbitrary address instead of
the
52txbuf address we trasmitted, so just check that txbuf is non NULL
53for success. This is ok because we open the SNP protocol in
54exclusive mode so we know we're the only ones transmitting on this
Here ---> ^
55box and since we only transmit one packet at a time we know our
56transmit was successfull.
57 */
58 if (txbuf)
59 {
60 dev->txbusy = 0;
61 break;
62 }
>if (net)
> {
>if (net->mode->state == GRUB_EFI_NETWORK_STOPPED
> @@ -192,13 +209,12 @@ open_card (struct grub_net_card *dev)
> efi_call_6 (net->receive_filters, net, filters, 0, 0, 0, NULL);
> }
>
> - efi_call_4 (grub_efi_system_table->boot_services->close_protocol,
> - dev->efi_net, &net_io_guid,
> - grub_efi_image_handle, dev->efi_handle);
>dev->efi_net = net;
> +} else {
> + return grub_error (GRUB_ERR_NET_NO_CARD, "%s: can't open protocol",
> + dev->name);
> }
>
> - /* If it failed we just try to run as best as we can */
>return GRUB_ERR_NONE;
> }
>
> @@ -208,8 +224,8 @@ close_card (struct grub_net_card *dev)
>efi_call_1 (dev->efi_net->shutdown, dev->efi_net);
>efi_call_1 (dev->efi_net->stop, dev->efi_net);
>efi_call_4 (grub_efi_system_table->boot_services->close_protocol,
> - dev->efi_net, &net_io_guid,
> - grub_efi_image_handle, dev->efi_handle);
> + dev->efi_handle, &n
Re: [PATCH 2/2] efi: library function grub_efi_close_protocol()
On Wed, Oct 06, 2021 at 10:30:43AM +0200, Heinrich Schuchardt wrote:
> Create a library function for CloseProtocol() and use it for the SNP
> driver.
>
> Signed-off-by: Heinrich Schuchardt
> ---
> grub-core/kern/efi/efi.c | 18 ++
> grub-core/net/drivers/efi/efinet.c | 8 ++--
> include/grub/efi/efi.h | 3 +++
> 3 files changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
> index 8cff7be02..1d331d858 100644
> --- a/grub-core/kern/efi/efi.c
> +++ b/grub-core/kern/efi/efi.c
> @@ -117,6 +117,24 @@ grub_efi_open_protocol (grub_efi_handle_t handle,
>return interface;
> }
>
> +grub_efi_status_t
> +grub_efi_close_protocol (grub_efi_handle_t handle,
> + grub_efi_guid_t *protocol)
grub_efi_close_protocol (grub_efi_handle_t handle, grub_efi_guid_t *protocol)
> +{
> + grub_efi_boot_services_t *b;
> + grub_efi_status_t status;
> +
> +
Please drop this redundant empty line...
> + b = grub_efi_system_table->boot_services;
I think you could do this above:
grub_efi_boot_services_t *b = grub_efi_system_table->boot_services;
> + status = efi_call_4 (b->close_protocol,
> +handle,
> +protocol,
> +grub_efi_image_handle,
> +0);
status = efi_call_4 (b->close_protocol, handle, protocol,
grub_efi_image_handle, NULL);
I am OK with lines a bit longer than 80 chars.
Otherwise LGTM.
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 0/4] Documentation improvements and fixes
On Fri, Nov 05, 2021 at 02:12:54PM -0500, Glenn Washburn wrote: > These patches are pretty self explanatory. The first patch was requested by > Daniel and Daniel suggested the link to the INSTALL file from the git web > repo. > > Glenn > > Glenn Washburn (4): > docs: Add sentence on where Debian packages can be searched for online > docs: Update development docs to include information on running test > suite > docs: Fix broken links in development docs > docs: Add documentation on packages for building documentation One nit, we should use "https://"; instead of "http://"; today. I will fix it before push. Otherwise for all patches Reviewed-by: Daniel Kiper Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 0/2] powerpc-ieee1275: support larger core.elf images
Hi Adrian, Thanks for testing! > root@ibook-g4-14:/home/glaubitz/grub# grub-install > --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. > grub-install: error: `/usr/local/lib/grub/powerpc-ieee1275/kernel.img' is > miscompiled: its start address is 0x20 instead of 0x80: ld.gold bug?. > root@ibook-g4-14:/home/glaubitz/grub# Fascinating. I saw a similar issue when I only updated the kernel (Makefile.core.def) and not the utils and fixed it with the change to offsets.h. Interestingly you're seeing the issue in reverse: the tools are expecting the new offset but /usr/local/lib/grub/powerpc-ieee1275/kernel.img is built with the old offset. Could you humour me and: - check that your kernel.img in /usr is indeed up to date - try grub-install ... -d ./grub-core? Many thanks! Kind regards, Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 03/19] mm: document grub internal memory management structures
Glenn Washburn writes:
> On Tue, 12 Oct 2021 18:29:52 +1100
> Daniel Axtens wrote:
>
>> I spent more than a trivial quantity of time figuring out pre_size and
>> whether a memory region's size contains the header cell or not.
>>
>> Document the meanings of all the properties. Hopefully now no-one else
>> has to figure it out!
>>
>> Signed-off-by: Daniel Axtens
>> ---
>> include/grub/mm_private.h | 21 +
>> 1 file changed, 21 insertions(+)
>>
>> diff --git a/include/grub/mm_private.h b/include/grub/mm_private.h
>> index c2c4cb1511c6..e80a059dd4e4 100644
>> --- a/include/grub/mm_private.h
>> +++ b/include/grub/mm_private.h
>> @@ -25,11 +25,18 @@
>> #define GRUB_MM_FREE_MAGIC 0x2d3c2808
>> #define GRUB_MM_ALLOC_MAGIC 0x6db08fa4
>>
>> +/* A header describing a block of memory - either allocated or free */
>> typedef struct grub_mm_header
>> {
>> + /* The 'next' free block in this region. A circular list.
>> + Only meaningful if the block is free.
>> + */
>>struct grub_mm_header *next;
>
> This and the subsequent comments do not follow the multiline comment
> style[1].
Will fix, thanks.
> One nit, "region. A circular" -> "region's circular".
Sure, will do.
> This comment leads me to wonder if the block is not free, what is the
> value of next? Seems like it should be NULL, if its not meaningful.
In practice the value will be:
- if the block was converted from an existing free block: whatever the
'next' pointer was when the block was free
- if the block was carved out of the middle or end of an existing free
block: whatever happened to be at that offset in memory.
We certainly _could_ null it out. I don't think there's any real value
in doing so but I'm open to being convinced. I suppose you could make an
argument that it reduces the chance that we'll follow a pointer to
somewhere random in memory but we check the magic before we check next
so we're fairly safe.
> https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Multi_002dLine-Comments
>
>> + /* The region size in cells (not bytes). Includes the header cell. */
>
> What exactly does "cell" mean in this context? I'm gathering cell is
> not defined as in this link[2], which is equivalent to "bit". Perhaps
> "size" should be renamed to "ncells" or something more descriptive.
>
> [2] https://en.wikipedia.org/wiki/Memory_cell_(computing)
The definition of cell is from mm.c:
The memory space is used as cells instead of bytes for simplicity. This
is important for some CPUs which may not access multiple bytes at a time
when the first byte is not aligned at a certain boundary (typically,
4-byte or 8-byte). The size of each cell is equal to the size of struct
grub_mm_header, so the header of each allocated/free block fits into one
cell precisely. One cell is 16 bytes on 32-bit platforms and 32 bytes
on 64-bit platforms.
I don't want to change variable names in this patch but I will update
the comment.
Kind regards,
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
