Re: [PATCH v21 33/33] docs: Document TPM2 key protector
On Mon, Nov 04, 2024 at 03:32:06PM +0800, Gary Lin wrote:
> Update the user manual to address TPM2 key protector including the two
> related commands, tpm2_key_protector_init and tpm2_key_protector_clear,
> and the user-space utility: grub-protect.
>
> Signed-off-by: Gary Lin
> ---
> docs/grub.texi | 512 +
> 1 file changed, 512 insertions(+)
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index fdd49d62e..64f10ee34 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
[...]
> +@node Invoking grub-protect
> +@section Invoking grub-protect
> +
> +The program @command{grub-protect} protects a disk encryption key with
> +a specified key protector.
> +
> +@table @option
> +@item --help
> +Print a summary of the command-line options and exit.
> +
> +@item --version
> +Print the version number of GRUB and exit.
> +
> +@item -a add|remove
> +@itemx --action=add|remove
> +Add or remove a key protector to or from a key.
> +
> +@item -p
s/-p/-p @var{protector}/
Please fix this minor issue and issues mentioned by Stefan. If you do that
feel free to add Reviewed-by: Daniel Kiper .
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v21 23/33] key_protector: Add TPM2 Key Protector
On Mon, Nov 04, 2024 at 01:04:56PM -0500, Stefan Berger wrote:
>
>
> On 11/4/24 2:31 AM, Gary Lin wrote:
> > From: Hernan Gatta
> >
> > The TPM2 key protector is a module that enables the automatic retrieval
> > of a fully-encrypted disk's unlocking key from a TPM 2.0.
> >
> > The theory of operation is such that the module accepts various
> > arguments, most of which are optional and therefore possess reasonable
> > defaults. One of these arguments is the keyfile/tpm2key parameter, which
> > is mandatory. There are two supported key formats:
> >
> > 1. Raw Sealed Key (--keyfile)
> > When sealing a key with TPM2_Create, the public portion of the sealed
> > key is stored in TPM2B_PUBLIC, and the private portion is in
> > TPM2B_PRIVATE. The raw sealed key glues the fully marshalled
> > TPM2B_PUBLIC and TPM2B_PRIVATE into one file.
> >
> > 2. TPM 2.0 Key (--tpm2key)
> > The following is the ASN.1 definition of TPM 2.0 Key File:
> >
> > TPMPolicy ::= SEQUENCE {
> > CommandCode [0] EXPLICIT INTEGER
> > CommandPolicy [1] EXPLICIT OCTET STRING
> > }
> >
> > TPMAuthPolicy ::= SEQUENCE {
> > Name[0] EXPLICIT UTF8STRING OPTIONAL
> > Policy [1] EXPLICIT SEQUENCE OF TPMPolicy
> > }
> >
> > TPMKey ::= SEQUENCE {
> > typeOBJECT IDENTIFIER
> > emptyAuth [0] EXPLICIT BOOLEAN OPTIONAL
> > policy [1] EXPLICIT SEQUENCE OF TPMPolicy OPTIONAL
> > secret [2] EXPLICIT OCTET STRING OPTIONAL
> > authPolicy [3] EXPLICIT SEQUENCE OF TPMAuthPolicy OPTIONAL
> > description [4] EXPLICIT UTF8String OPTIONAL,
> > rsaParent [5] EXPLICIT BOOLEAN OPTIONAL,
> > parent INTEGER
> > pubkey OCTET STRING
> > privkey OCTET STRING
> > }
> >
> >The TPM2 key protector only expects a "sealed" key in DER encoding,
> >so 'type' is always 2.23.133.10.1.5, 'emptyAuth' is 'TRUE', and
> >'secret' is empty. 'policy' and 'authPolicy' are the possible policy
> >command sequences to construst the policy digest to unseal the key.
> >Similar to the raw sealed key, the public portion (TPM2B_PUBLIC) of
> >the sealed key is stored in 'pubkey', and the private portion
> >(TPM2B_PRIVATE) is in 'privkey'.
> >
> >For more details:
> > https://www.hansenpartnership.com/draft-bottomley-tpm2-keys.html
> >
> > This sealed key file is created via the grub-protect tool. The tool
> > utilizes the TPM's sealing functionality to seal (i.e., encrypt) an
> > unlocking key using a Storage Root Key (SRK) to the values of various
> > Platform Configuration Registers (PCRs). These PCRs reflect the state
> > of the system as it boots. If the values are as expected, the system
> > may be considered trustworthy, at which point the TPM allows for a
> > caller to utilize the private component of the SRK to unseal (i.e.,
> > decrypt) the sealed key file. The caller, in this case, is this key
> > protector.
> >
> > The TPM2 key protector registers two commands:
> >
> > - tpm2_key_protector_init: Initializes the state of the TPM2 key
> > protector for later usage, clearing any
> > previous state, too, if any.
> >
> > - tpm2_key_protector_clear: Clears any state set by tpm2_key_protector_init.
> >
> > The way this is expected to be used requires the user to, either
> > interactively or, normally, via a boot script, initialize/configure
> > the key protector and then specify that it be used by the 'cryptomount'
> > command (modifications to this command are in a different patch).
> >
> > For instance, to unseal the raw sealed key file:
> >
> > tpm2_key_protector_init --keyfile=(hd0,gpt1)/efi/grub/sealed-1.key
> > cryptomount -u -P tpm2
> >
> > tpm2_key_protector_init --keyfile=(hd0,gpt1)/efi/grub/sealed-2.key
> > --pcrs=7,11
> > cryptomount -u -P tpm2
> >
> > Or, to unseal the TPM 2.0 Key file:
> >
> > tpm2_key_protector_init --tpm2key=(hd0,gpt1)/efi/grub/sealed-1.tpm
> > cryptomount -u -P tpm2
> >
> > tpm2_key_protector_init --tpm2key=(hd0,gpt1)/efi/grub/sealed-2.tpm
> > --pcrs=7,11
> > cryptomount -u -P tpm2
> >
> > If a user does not initialize the key protector and attempts to use it
> > anyway, the protector returns an error.
> >
> > Before unsealing the key, the TPM2 key protector follows the "TPMPolicy"
> > sequences to enforce the TPM policy commands to construct a valid policy
> > digest to unseal the key.
> >
> > For the TPM 2.0 Key files, 'authPolicy' may contain multiple "TPMPolicy"
> > sequences, the TPM2 key protector iterates 'authPolicy' to find a valid
> > sequence to unseal key. If 'authPolicy' is empty or all sequences in
> > 'authPolicy' fail, the protector tries the one from 'policy'. In case
> > 'policy' is also empty, the protector creates a "TPMPolicy" sequence
> > based on the given PCR selection.
> >
> > For the raw sealed key, the TPM2 key protector treats the key file as a
> > T
Re: [PATCH v21 25/33] util/grub-protect: Add new tool
On Mon, Nov 04, 2024 at 01:11:08PM -0500, Stefan Berger wrote:
>
>
> On 11/4/24 2:31 AM, Gary Lin wrote:
> > From: Hernan Gatta
> >
> > To utilize the key protectors framework, there must be a way to protect
> > full-disk encryption keys in the first place. The grub-protect tool
> > includes support for the TPM2 key protector but other protectors that
> > require setup ahead of time can be supported in the future.
> >
> > For the TPM2 key protector, the intended flow is for a user to have a
> > LUKS 1 or LUKS 2-protected fully-encrypted disk. The user then creates a
> > new LUKS key file, say by reading /dev/urandom into a file, and creates
> > a new LUKS key slot for this key. Then, the user invokes the grub-protect
> > tool to seal this key file to a set of PCRs using the system's TPM 2.0.
> > The resulting sealed key file is stored in an unencrypted partition such
> > as the EFI System Partition (ESP) so that GRUB may read it. The user also
> > has to ensure the cryptomount command is included in GRUB's boot script
> > and that it carries the requisite key protector (-P) parameter.
> >
> > Sample usage:
> >
> > $ dd if=/dev/urandom of=luks-key bs=1 count=32
> > $ sudo cryptsetup luksAddKey /dev/sdb1 luks-key --pbkdf=pbkdf2 --hash=sha512
> >
> > To seal the key with TPM 2.0 Key File (recommended):
> >
> > $ sudo grub-protect --action=add \
> > --protector=tpm2 \
> > --tpm2-pcrs=0,2,4,7,9 \
> > --tpm2key \
> > --tpm2-keyfile=luks-key \
> > --tpm2-outfile=/boot/efi/efi/grub/sealed.tpm
> >
> > Or, to seal the key with the raw sealed key:
> >
> > $ sudo grub-protect --action=add \
> > --protector=tpm2 \
> > --tpm2-pcrs=0,2,4,7,9 \
> > --tpm2-keyfile=luks-key \
> > --tpm2-outfile=/boot/efi/efi/grub/sealed.key
> >
> > Then, in the boot script, for TPM 2.0 Key File:
> >
> > tpm2_key_protector_init --tpm2key=(hd0,gpt1)/efi/grub/sealed.tpm
> > cryptomount -u -P tpm2
> >
> > Or, for the raw sealed key:
> >
> > tpm2_key_protector_init --keyfile=(hd0,gpt1)/efi/grub/sealed.key
> > --pcrs=0,2,4,7,9
> > cryptomount -u -P tpm2
> >
> > The benefit of using TPM 2.0 Key File is that the PCR set is already
> > written in the key file, so there is no need to specify PCRs when
> > invoking tpm2_key_protector_init.
> >
> > Cc: Stefan Berger
> > Signed-off-by: Hernan Gatta
> > Signed-off-by: Gary Lin
> > ---
> > .gitignore|2 +
> > Makefile.util.def | 26 +
> > configure.ac | 30 +
> > docs/man/grub-protect.h2m |4 +
> > util/grub-protect.c | 1407 +
> > 5 files changed, 1469 insertions(+)
> > create mode 100644 docs/man/grub-protect.h2m
> > create mode 100644 util/grub-protect.c
> >
> > diff --git a/.gitignore b/.gitignore
> > index 4c1f91db8..2105d87c8 100644
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -169,6 +169,8 @@ widthspec.bin
> > /grub-ofpathname.exe
> > /grub-probe
> > /grub-probe.exe
> > +/grub-protect
> > +/grub-protect.exe
> > /grub-reboot
> > /grub-render-label
> > /grub-render-label.exe
> > diff --git a/Makefile.util.def b/Makefile.util.def
> > index fb82f59a0..074c0aff7 100644
> > --- a/Makefile.util.def
> > +++ b/Makefile.util.def
> > @@ -208,6 +208,32 @@ program = {
> > ldadd = '$(LIBINTL) $(LIBDEVMAPPER) $(LIBZFS) $(LIBNVPAIR) $(LIBGEOM)';
> > };
> > +program = {
> > + name = grub-protect;
> > + mansection = 1;
> > +
> > + common = grub-core/kern/emu/argp_common.c;
> > + common = grub-core/osdep/init.c;
> > + common = grub-core/lib/tss2/buffer.c;
> > + common = grub-core/lib/tss2/tss2_mu.c;
> > + common = grub-core/lib/tss2/tpm2_cmd.c;
> > + common = grub-core/commands/tpm2_key_protector/args.c;
> > + common = grub-core/commands/tpm2_key_protector/tpm2key_asn1_tab.c;
> > + common = util/grub-protect.c;
> > + common = util/probe.c;
> > +
> > + cflags = '-I$(srcdir)/grub-core/lib/tss2
> > -I$(srcdir)/grub-core/commands/tpm2_key_protector';
> > +
> > + ldadd = libgrubmods.a;
> > + ldadd = libgrubgcry.a;
> > + ldadd = libgrubkern.a;
> > + ldadd = grub-core/lib/gnulib/libgnu.a;
> > + ldadd = '$(LIBTASN1)';
> > + ldadd = '$(LIBINTL) $(LIBDEVMAPPER) $(LIBUTIL) $(LIBZFS) $(LIBNVPAIR)
> > $(LIBGEOM)';
> > +
> > + condition = COND_GRUB_PROTECT;
> > +};
> > +
> > program = {
> > name = grub-mkrelpath;
> > mansection = 1;
> > diff --git a/configure.ac b/configure.ac
> > index 458b8382b..ad1e7bea5 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -76,6 +76,7 @@ grub_TRANSFORM([grub-mkpasswd-pbkdf2])
> > grub_TRANSFORM([grub-mkrelpath])
> > grub_TRANSFORM([grub-mkrescue])
> > grub_TRANSFORM([grub-probe])
> > +grub_TRANSFORM([grub-protect])
> > grub_TRANSFORM([grub-reboot])
> > grub_TRANSFORM([grub-script-check])
> > grub_TRANSFORM([grub-se
Re: [PATCH v21 21/33] tss2: Add TPM2 types and Marshal/Unmarshal functions
On Tue, Nov 05, 2024 at 10:58:47AM +0800, Gary Lin wrote: > On Mon, Nov 04, 2024 at 01:20:45PM -0500, Stefan Berger wrote: > > > > > > On 11/4/24 2:31 AM, Gary Lin via Grub-devel wrote: > > > This commit adds the necessary TPM2 types and structs as the preparation > > > for the TPM2 Software Stack (TSS2) support. The Marshal/Unmarshal > > > functions are also added to handle the data structure to be submitted to > > > TPM2 commands and to be received from the response. > > > > > > Cc: Stefan Berger > > > Signed-off-by: Hernan Gatta > > > Signed-off-by: Gary Lin > > > Reviewed-by: Daniel Kiper > > > --- > > > grub-core/lib/tss2/tss2_mu.c | 1174 + > > > grub-core/lib/tss2/tss2_mu.h | 397 ++ > > > grub-core/lib/tss2/tss2_structs.h | 796 +++ > > > grub-core/lib/tss2/tss2_types.h | 404 ++ > > > 4 files changed, 2771 insertions(+) > > > create mode 100644 grub-core/lib/tss2/tss2_mu.c > > > create mode 100644 grub-core/lib/tss2/tss2_mu.h > > > create mode 100644 grub-core/lib/tss2/tss2_structs.h > > > create mode 100644 grub-core/lib/tss2/tss2_types.h > > > > > > > > + > > > +/* Buffer Size Constants */ > > > +#define TPM_MAX_PCRS 32 > > > > This should be 24 and it seems that it can be changed easily. > > > It is from tpm2-tss: > https://github.com/tpm2-software/tpm2-tss/blob/master/include/tss2/tss2_tpm2_types.h#L25 > > I'm still trying to find out where the number is from. > tpm2-tss set the value without a clear explanation: https://github.com/tpm2-software/tpm2-tss/commit/35750de13af0b2aa4e82e24308a99c7e0f06c961#diff-2540f5a71a43e69031c4dcb091bc9356f54d0936a5f31bdc8b8d4d07612c419eR50 There were IMPLEMENTATION_PCR(24) and PLATFORM_PCR(24) to indicate the number of PCRs. Those two constants were removed and then MAX_PCRS was set as 32 in the commit. On the other hand, the TPM 2.0 library spec only mentions that the platform specific spec can define the minimum number of PCR, and "32" is only used as an example for TPMS_PCR_SELECTION. EXAMPLE 3 If the applicable platform-specific specification requires that the TPM have a minimum of 24 PCR but the TPM implements 32, then a PCR select of 3 octets would imply that PCR 24-31 are not selected. In "4.6 PCR Requirement" of "TCG PC Client Platform TPM Profile Specification for TPM 2.0", it defines the minimum number of PCRs is 24. A conformant TPM SHALL allow an allocation of a minimum of 24 PCRs, 0-23, within all allocated bank. It seems to me that there is no static maximum number of PCRs is defined. Anyway, I'll change TPM_MAX_PCRS to 24. For bootloaders, only the PCRs for SRTM, i.e. 0~15, really matter, so supporting 24 PCRs is enough. Gary Lin ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3 01/16] ieee1275/openfw: IBM client architecture(CAS) reboot support
On 2024-10-11 03:13, grub-devel-requ...@gnu.org wrote:
Message: 1
Date: Thu, 10 Oct 2024 15:43:19 -0600
From: Leo Sandoval
To: grub-devel@gnu.org
Subject: [PATCH v3 01/16] ieee1275/openfw: IBM client architecture
(CAS) reboot support
Message-ID: <20241010214334.1749167-2-lsand...@redhat.com>
Content-Type: text/plain; charset="US-ASCII"; x-default=true
From: Paulo Flabiano Smorigo
This is an implementation of IBM client architecture (CAS) reboot for
GRUB.
There are cases where the POWER firmware must reboot in order to
support
specific features requested by a kernel. The kernel calls
ibm,client-architecture-support and it may either return or reboot with
the new feature set. eg:
Calling ibm,client-architecture-support.../
Elapsed time since release of system processors: 70959 mins 50 secs
Welcome to GRUB!
Instead of return to the GRUB menu, it will check if the flag for CAS
reboot is set. If so, grub will automatically boot the last booted
kernel using the same parameters
Signed-off-by: Paulo Flabiano Smorigo
[rharw...@redhat.com: commit message rewrap]
Signed-off-by: Robbie Harwood
---
grub-core/kern/ieee1275/openfw.c | 63
grub-core/normal/main.c | 19 ++
grub-core/script/execute.c | 7
include/grub/ieee1275/ieee1275.h | 2 +
4 files changed, 91 insertions(+)
diff --git a/grub-core/kern/ieee1275/openfw.c
b/grub-core/kern/ieee1275/openfw.c
index 11b2beb2f..e2ecc65d2 100644
--- a/grub-core/kern/ieee1275/openfw.c
+++ b/grub-core/kern/ieee1275/openfw.c
@@ -591,3 +591,66 @@ grub_ieee1275_get_boot_dev (void)
return bootpath;
}
+
+/* Check if it's a CAS reboot. If so, set the script to be executed.
*/
+int
+grub_ieee1275_cas_reboot (char *script)
+{
+ grub_uint32_t ibm_ca_support_reboot;
+ grub_uint32_t ibm_fw_nbr_reboots;
+ char property_value[10];
+ grub_ssize_t actual;
+ grub_ieee1275_ihandle_t options;
+
+ if (grub_ieee1275_finddevice ("/options", &options) < 0)
+return -1;
+
+ /* Check two properties, one is enough to get cas reboot value */
+ ibm_ca_support_reboot = 0;
+ if (grub_ieee1275_get_integer_property (grub_ieee1275_chosen,
+
"ibm,client-architecture-support-reboot",
+ &ibm_ca_support_reboot,
+ sizeof
(ibm_ca_support_reboot),
+ &actual) >= 0)
+grub_dprintf("ieee1275", "ibm,client-architecture-support-reboot:
%u\n",
+ ibm_ca_support_reboot);
+
+ ibm_fw_nbr_reboots = 0;
+ if (grub_ieee1275_get_property (options, "ibm,fw-nbr-reboots",
+ property_value, sizeof
(property_value),
+ &actual) >= 0)
+{
+ property_value[sizeof (property_value) - 1] = 0;
+ ibm_fw_nbr_reboots = (grub_uint8_t) grub_strtoul
(property_value, 0, 10);
+ grub_dprintf("ieee1275", "ibm,fw-nbr-reboots: %u\n",
ibm_fw_nbr_reboots);
+}
+
+ if (ibm_ca_support_reboot || ibm_fw_nbr_reboots)
+{
+ if (! grub_ieee1275_get_property_length (options,
"boot-last-label", &actual))
+{
+ if (actual > 1024)
+script = grub_realloc (script, actual + 1);
+ grub_ieee1275_get_property (options, "boot-last-label",
script, actual,
+ &actual);
+ return 0;
+}
+}
+
+ grub_ieee1275_set_boot_last_label ("");
+
+ return -1;
+}
+
+int grub_ieee1275_set_boot_last_label (const char *text)
+{
+ grub_ieee1275_ihandle_t options;
+ grub_ssize_t actual;
+
+ grub_dprintf("ieee1275", "set boot_last_label (size: %u)\n",
grub_strlen(text));
+ if (! grub_ieee1275_finddevice ("/options", &options) &&
+ options != (grub_ieee1275_ihandle_t) -1)
+grub_ieee1275_set_property (options, "boot-last-label", text,
+grub_strlen (text), &actual);
+ return 0;
+}
diff --git a/grub-core/normal/main.c b/grub-core/normal/main.c
index bd4431000..d3f53d93d 100644
--- a/grub-core/normal/main.c
+++ b/grub-core/normal/main.c
@@ -34,6 +34,9 @@
#include
#include
#include
+#ifdef GRUB_MACHINE_IEEE1275
+#include
+#endif
GRUB_MOD_LICENSE ("GPLv3+");
@@ -276,6 +279,22 @@ grub_normal_execute (const char *config, int
nested, int batch)
{
menu = read_config_file (config);
+#ifdef GRUB_MACHINE_IEEE1275
+ int boot;
+ boot = 0;
+ char *script;
+ script = grub_malloc (1024);
+ if (! grub_ieee1275_cas_reboot (script))
+{
+ char *dummy[1] = { NULL };
+ if (! grub_script_execute_sourcecode (script))
+boot = 1;
+}
+ grub_free (script);
+ if (boot)
+grub_command_execute ("boot", 0, 0);
+#endif
+
/* Ignore any error. */
grub_errno = GRUB_ERR_NONE;
}
diff --git a/grub-core/script/execute.c b/grub-core/script/execute.c
index 14ff09094..dab8fd2ae 100644
--- a/grub-core/script/execute.c
+++ b/g
Re: [PATCH v21 25/33] util/grub-protect: Add new tool
On Mon, Nov 04, 2024 at 03:31:58PM +0800, Gary Lin wrote:
> From: Hernan Gatta
>
> To utilize the key protectors framework, there must be a way to protect
> full-disk encryption keys in the first place. The grub-protect tool
> includes support for the TPM2 key protector but other protectors that
> require setup ahead of time can be supported in the future.
>
> For the TPM2 key protector, the intended flow is for a user to have a
> LUKS 1 or LUKS 2-protected fully-encrypted disk. The user then creates a
> new LUKS key file, say by reading /dev/urandom into a file, and creates
> a new LUKS key slot for this key. Then, the user invokes the grub-protect
> tool to seal this key file to a set of PCRs using the system's TPM 2.0.
> The resulting sealed key file is stored in an unencrypted partition such
> as the EFI System Partition (ESP) so that GRUB may read it. The user also
> has to ensure the cryptomount command is included in GRUB's boot script
> and that it carries the requisite key protector (-P) parameter.
>
> Sample usage:
>
> $ dd if=/dev/urandom of=luks-key bs=1 count=32
> $ sudo cryptsetup luksAddKey /dev/sdb1 luks-key --pbkdf=pbkdf2 --hash=sha512
>
> To seal the key with TPM 2.0 Key File (recommended):
>
> $ sudo grub-protect --action=add \
> --protector=tpm2 \
> --tpm2-pcrs=0,2,4,7,9 \
> --tpm2key \
> --tpm2-keyfile=luks-key \
> --tpm2-outfile=/boot/efi/efi/grub/sealed.tpm
>
> Or, to seal the key with the raw sealed key:
>
> $ sudo grub-protect --action=add \
> --protector=tpm2 \
> --tpm2-pcrs=0,2,4,7,9 \
> --tpm2-keyfile=luks-key \
> --tpm2-outfile=/boot/efi/efi/grub/sealed.key
>
> Then, in the boot script, for TPM 2.0 Key File:
>
> tpm2_key_protector_init --tpm2key=(hd0,gpt1)/efi/grub/sealed.tpm
> cryptomount -u -P tpm2
>
> Or, for the raw sealed key:
>
> tpm2_key_protector_init --keyfile=(hd0,gpt1)/efi/grub/sealed.key
> --pcrs=0,2,4,7,9
> cryptomount -u -P tpm2
>
> The benefit of using TPM 2.0 Key File is that the PCR set is already
> written in the key file, so there is no need to specify PCRs when
> invoking tpm2_key_protector_init.
>
> Cc: Stefan Berger
> Signed-off-by: Hernan Gatta
> Signed-off-by: Gary Lin
> ---
> .gitignore|2 +
> Makefile.util.def | 26 +
> configure.ac | 30 +
> docs/man/grub-protect.h2m |4 +
> util/grub-protect.c | 1407 +
> 5 files changed, 1469 insertions(+)
> create mode 100644 docs/man/grub-protect.h2m
> create mode 100644 util/grub-protect.c
>
> diff --git a/.gitignore b/.gitignore
> index 4c1f91db8..2105d87c8 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -169,6 +169,8 @@ widthspec.bin
> /grub-ofpathname.exe
> /grub-probe
> /grub-probe.exe
> +/grub-protect
> +/grub-protect.exe
> /grub-reboot
> /grub-render-label
> /grub-render-label.exe
> diff --git a/Makefile.util.def b/Makefile.util.def
> index fb82f59a0..074c0aff7 100644
> --- a/Makefile.util.def
> +++ b/Makefile.util.def
> @@ -208,6 +208,32 @@ program = {
>ldadd = '$(LIBINTL) $(LIBDEVMAPPER) $(LIBZFS) $(LIBNVPAIR) $(LIBGEOM)';
> };
>
> +program = {
> + name = grub-protect;
> + mansection = 1;
> +
> + common = grub-core/kern/emu/argp_common.c;
> + common = grub-core/osdep/init.c;
> + common = grub-core/lib/tss2/buffer.c;
> + common = grub-core/lib/tss2/tss2_mu.c;
> + common = grub-core/lib/tss2/tpm2_cmd.c;
> + common = grub-core/commands/tpm2_key_protector/args.c;
> + common = grub-core/commands/tpm2_key_protector/tpm2key_asn1_tab.c;
> + common = util/grub-protect.c;
> + common = util/probe.c;
> +
> + cflags = '-I$(srcdir)/grub-core/lib/tss2
> -I$(srcdir)/grub-core/commands/tpm2_key_protector';
> +
> + ldadd = libgrubmods.a;
> + ldadd = libgrubgcry.a;
> + ldadd = libgrubkern.a;
> + ldadd = grub-core/lib/gnulib/libgnu.a;
> + ldadd = '$(LIBTASN1)';
> + ldadd = '$(LIBINTL) $(LIBDEVMAPPER) $(LIBUTIL) $(LIBZFS) $(LIBNVPAIR)
> $(LIBGEOM)';
> +
> + condition = COND_GRUB_PROTECT;
> +};
> +
> program = {
>name = grub-mkrelpath;
>mansection = 1;
> diff --git a/configure.ac b/configure.ac
> index 458b8382b..ad1e7bea5 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -76,6 +76,7 @@ grub_TRANSFORM([grub-mkpasswd-pbkdf2])
> grub_TRANSFORM([grub-mkrelpath])
> grub_TRANSFORM([grub-mkrescue])
> grub_TRANSFORM([grub-probe])
> +grub_TRANSFORM([grub-protect])
> grub_TRANSFORM([grub-reboot])
> grub_TRANSFORM([grub-script-check])
> grub_TRANSFORM([grub-set-default])
> @@ -2068,6 +2069,29 @@ fi
> AC_SUBST([LIBZFS])
> AC_SUBST([LIBNVPAIR])
>
> +AC_ARG_ENABLE([grub-protect],
> + [AS_HELP_STRING([--enable-grub-protect],
> + [build and install the `grub-protect' utility
> (default=guessed)])])
> +if test x"$enable_grub_protect" = xno ; then
> + grub_protect_excuse="ex
Re: [PATCH V6] ieee1275/ofdisk: retry on open and read failure
On 2024-06-26 16:19, Mukesh Kumar Chaurasiya wrote:
Sometimes, when booting from a very busy SAN, the access to the
disk can fail and then GRUB will eventually drop to GRUB prompt.
This scenario is more frequent when deploying many machines at
the same time using the same SAN.
This patch aims to force the ofdisk module to retry the open or
read function for network disks after it fails. We use
DEFAULT_RETRY_TIMEOUT, which is 15 seconds to specify the time it'll
retry to access the disk before it definitely fails. The timeout can be
changed by setting the environment variable ofdisk_retry_timeout.
If the environment variable fails to read, GRUB will consider the
default value of 15 seconds.
Signed-off-by: Diego Domingos
Signed-off-by: Mukesh Kumar Chaurasiya
---
docs/grub.texi | 8
grub-core/disk/ieee1275/ofdisk.c | 82 ++--
2 files changed, 87 insertions(+), 3 deletions(-)
diff --git a/docs/grub.texi b/docs/grub.texi
index f3bdc2564..9514271fc 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -3308,6 +3308,7 @@ These variables have special meaning to GRUB.
* net_default_ip::
* net_default_mac::
* net_default_server::
+* ofdisk_retry_timeout::
* pager::
* prefix::
* pxe_blksize::
@@ -3738,6 +3739,13 @@ The default is the value of @samp{color_normal}
(@pxref{color_normal}).
@xref{Network}.
+@node ofdisk_retry_timeout
+@subsection ofdisk_retry_timeout
+
+The time in seconds till which the GRUB will retry to open or read a
disk in
+case of failure to do so. This value defaults to 15 seconds.
+
+
@node pager
@subsection pager
diff --git a/grub-core/disk/ieee1275/ofdisk.c
b/grub-core/disk/ieee1275/ofdisk.c
index c6cba0c8a..675e039c5 100644
--- a/grub-core/disk/ieee1275/ofdisk.c
+++ b/grub-core/disk/ieee1275/ofdisk.c
@@ -24,6 +24,9 @@
#include
#include
#include
+#include
+
+#define RETRY_DEFAULT_TIMEOUT 15
static char *last_devpath;
static grub_ieee1275_ihandle_t last_ihandle;
@@ -452,7 +455,7 @@ compute_dev_path (const char *name)
}
static grub_err_t
-grub_ofdisk_open (const char *name, grub_disk_t disk)
+grub_ofdisk_open_real (const char *name, grub_disk_t disk)
{
grub_ieee1275_phandle_t dev;
char *devpath;
@@ -525,6 +528,54 @@ grub_ofdisk_open (const char *name, grub_disk_t
disk)
return 0;
}
+static grub_uint64_t
+grub_ofdisk_disk_timeout (grub_disk_t disk)
+{
+ grub_uint64_t retry = RETRY_DEFAULT_TIMEOUT;
+ const char *timeout = grub_env_get ("ofdisk_retry_timeout");
+ const char *timeout_end;
+
+ if (grub_strstr (disk->name, "fibre-channel") != NULL ||
+ grub_strstr (disk->name, "vfc-client") != NULL)
+{
+ if (timeout == NULL)
+ return retry;
+ retry = grub_strtoul (timeout, &timeout_end, 10);
+ /* Ignore all errors and return default timeout */
+ if (*timeout == '\0' ||
+ *timeout_end != '\0')
+ return RETRY_DEFAULT_TIMEOUT;
+}
+ else
+return 0;
+
+ return retry;
+}
+
+static grub_err_t
+grub_ofdisk_open (const char *name, grub_disk_t disk)
+{
+ grub_err_t err;
+ grub_uint64_t timeout = grub_get_time_ms () +
(grub_ofdisk_disk_timeout (disk) * 1000);
+ grub_uint16_t inc = 0;
+
+ do
+{
+ err = grub_ofdisk_open_real (name, disk);
+ if (err == GRUB_ERR_UNKNOWN_DEVICE)
+ grub_dprintf ("ofdisk", "Failed to open disk %s.\n", name);
+ if (grub_get_time_ms () >= timeout)
+break;
+ grub_dprintf ("ofdisk", "Retry to open disk %s.\n", name);
+ /*
+ * Increase wait time for subsequent requests
+ * Cur time is used as a source of randomness
+ */
+ grub_millisleep ((32 << ++inc) * (grub_get_time_ms () % 32));
+} while (1);
+ return err;
+}
+
static void
grub_ofdisk_close (grub_disk_t disk)
{
@@ -568,8 +619,8 @@ grub_ofdisk_prepare (grub_disk_t disk,
grub_disk_addr_t sector)
}
static grub_err_t
-grub_ofdisk_read (grub_disk_t disk, grub_disk_addr_t sector,
- grub_size_t size, char *buf)
+grub_ofdisk_read_real (grub_disk_t disk, grub_disk_addr_t sector,
+ grub_size_t size, char *buf)
{
grub_err_t err;
grub_ssize_t actual;
@@ -587,6 +638,31 @@ grub_ofdisk_read (grub_disk_t disk,
grub_disk_addr_t sector,
return 0;
}
+static grub_err_t
+grub_ofdisk_read (grub_disk_t disk, grub_disk_addr_t sector,
+ grub_size_t size, char *buf)
+{
+ grub_err_t err;
+ grub_uint64_t timeout = grub_get_time_ms () +
(grub_ofdisk_disk_timeout (disk) * 1000);
+ grub_uint16_t inc = 0;
+
+ do
+{
+ err = grub_ofdisk_read_real (disk, sector, size, buf);
+ if (err == GRUB_ERR_UNKNOWN_DEVICE)
+ grub_dprintf ("ofdisk", "Failed to read disk %s.\n",
(char*)disk->data);
+ if (grub_get_time_ms () >= timeout)
+break;
+ grub_dprintf ("ofdisk", "Retry to read disk %s.\n",
(char*)disk->data);
+ /*
+ * Increase wait time for subsequent requests
+ * Cur time is used as a source of rando
Re: [PATCH v21 33/33] docs: Document TPM2 key protector
On Mon, Nov 04, 2024 at 12:42:19PM -0500, Stefan Berger wrote:
>
>
> On 11/4/24 2:32 AM, Gary Lin via Grub-devel wrote:
> > Update the user manual to address TPM2 key protector including the two
> > related commands, tpm2_key_protector_init and tpm2_key_protector_clear,
> > and the user-space utility: grub-protect.
> >
> > Signed-off-by: Gary Lin
> > ---
> > docs/grub.texi | 512 +
> > 1 file changed, 512 insertions(+)
> >
> > diff --git a/docs/grub.texi b/docs/grub.texi
> > index fdd49d62e..64f10ee34 100644
> > --- a/docs/grub.texi
> > +++ b/docs/grub.texi
> > @@ -6443,6 +6443,8 @@ you forget a command, you can run the command
> > @command{help}
> > * smbios:: Retrieve SMBIOS information
> > * source:: Read a configuration file in same context
> > * test::Check file types and compare values
> > +* tpm2_key_protector_init:: Initialize the TPM2 key protector
> > +* tpm2_key_protector_clear::Clear the TPM2 key protector
> > * true::Do nothing, successfully
> > * trust:: Add public key to list of trusted keys
> > * unset:: Unset an environment variable
> > @@ -8001,6 +8003,56 @@ either @var{expression1} or @var{expression2} is true
> > @end table
> > @end deffn
> > +@node tpm2_key_protector_init
> > +@subsection tpm2_key_protector_init
> > +
> > +@deffn Command tpm2_key_protector_init [@option{-m} mode] | [@option{-p}
> > pcrlist] | [@option{-b} pcrbank] | [ [@option{-T} tpm2key_file] |
> > [@option{-k} keyfile] ] | [@option{-s} handle] | [@option{-a} srk_type] |
> > [@option{-n} nv_index]
> > +Initialize the TPM2 key protector to unseal the key for the
> > @command{cryptomount}
> > +(@pxref{cryptomount}) command. There are two supported modes,
> > +SRK(@kbd{srk}) and NV index(@kbd{nv}), to be specified by the option
>
> For the SRK you should at least once mention that it is in the owner
> hierarchy. If this hierachy has a password you may need additional
> parameters. Once I ran the following command I unfortunately cannot use this
> tool anymore and an previously setup key doesn't load while in grub:
>
> sudo tpm2_changeauth -c owner newpass
>
> The owner hiarchy is now password-protected.
>
> To remove the password:
>
> sudo tpm2_changeauth -c owner -p newpass
>
> If password protection of the owner hierarchy is not supported then you
> should probably also mention this.
>
Good point. The goal of this patchset is to unlock the disk
automatically, so it doesn't expect the password for the owner
hierarchy, at least for now. I'll add a few more words to address the
issue.
>
> > +@option{-m}. The default mode is SRK. The main difference between SRK mode
> > +and NV index mode is the storage of the sealed key. For SRK mode, the
> > sealed
> > +key is stored in a file while NV index mode stores the sealed key in the
> > +non-volatile memory inside TPM with a given NV index.
> > +
> > +The @option{-p} and @option{-b} options are used to supply the PCR list and
> > +bank that the key is sealed with. The PCR list is a comma-separated list,
> > e.g.,
> > +'0,2,4,7,9', to represent the involved PCRs, and the default is '7'. The
> > PCR
> > +bank is chosen by selecting a hash algorithm. The current supported PCR
> > banks
> > +are SHA1, SHA256, SHA384, and SHA512, and the default is SHA256.
> > +
> > +There are some options only available for the specific mode. The
> > SRK-specific
>
>
> Some options are only available for a specific mode.
>
Will fix it in the next version.
>
> > +options are @option{-T}, @option{-k}, @option{-a}, and @option{-s}. On the
> > +other hand, the NV index-specific option is @option{-n}.
> > +
> > +The key file for SRK mode can be supplied with either @option{-T} or
> > +@option{-k}. The @option{-T} option is for the path to the key file in
> > +TPM 2.0 Key File format. Since the parameters for the TPM commands are
> > written
> > +in the file, there is no need to set the PCR list(@option{-p}) and
> > +bank(@option{-b}) when using the @option{-T} option. The @option{-k} option
> > +is for the key file in the raw format, and the @option{-p} and @option{-b}
> > +options are necessary for the non-default PCR list or bank.
> > +
> > +Besides the key file, there are two options, @option{-a} and @option{-s},
> > to
> > +tweak the TPM Storage Root Key (SRK). The SRK can be either created at
> > +runtime or stored in the non-volatile memory. When creating SRK at runtime,
> > +GRUB provides the SRK template to TPM to create the key. There are two SRK
>
> ... to the TPM to ...
>
Will fix it in the next version.
> > +templates for the @option{-a} option, ECC and RSA, and the default is ECC.
> > +If the SRK is stored in a specific handle, e.g. @code{0x8101}, the
> > +@option{-s} option can be used to set the handle to notify GRUB to load
> > +the SRK from the
Re: default entry different from timeout entry
Le lun. 14 oct. 2024, 00:13, Samuel Thibault a écrit : > Hello, > > For the debian installation CD, when using syslinux we can set an > on-timeout entry which is different from the default entry. This is > quite important because we want sighted people to be able to just press > enter to get the sighted installer, and non-sighted people to possibly > just have to wait before getting a self-speaking installer. > > Would it be ok to implement this in grub? > How do you inform the user about this arrangement? For sighted people there is expectation that highlighted entry will be booted automatically and also that highlighted entry is booted on enter. It's probably just a UI problem though and it's just a question of correct communication. > > Samuel > > ___ > 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: [External] : Re: [PATCH] Allow background to be set when theme is set
Hey, So a few suggestions. You should use git send-email to send your patch. This will format the patch nicely and make it easier to apply your patch to the rest of the GRUB code. Also, when sending an update of your patch, instead of replying to the initial patch, you should send a separate email with an updated version number, i.e. [PATCH v2]. You will also need a signed-off-by at the end of your commit message for the patch. Alec Brown On Tue, Oct 22, 2024 at 2:50 PM, Lin wrote: > On 13.06.24 17:28, Lin wrote: > > > > Hello, first time contributor here. > > I'm not experienced with mailing lists and mailing patches so just let > > me know if I do anything wrong :) > > > > This patch simply allows that if GRUB_BACKGROUND and GRUB_THEME is set > > at the same time, both pieces of code gets generated into grub.cfg by > > grub-mkconfig. > > > > If a theme is loaded, the background image will be visible inside the > > console. > > > Already made my first mistake and got my patch reversed lol. Corrected > patch is below. > > Again, when setting a GRUB_THEME, the elif disallows the GRUB_BACKGROUND > to be used, yet with my patch applied a set background image will not > interfere with the theme, but only be visible as the background for the > console when opened with 'c'. > > Thanks, > Lin > > > diff --git a/util/grub.d/00_header.in b/util/grub.d/00_header.in > index f86b69bad..4e8f25702 100644 > --- a/util/grub.d/00_header.in > +++ b/util/grub.d/00_header.in > @@ -263,7 +263,9 @@ EOF > set theme=(\$root)`make_system_path_relative_to_its_root $GRUB_THEME` > export theme > EOF > - elif [ "x$GRUB_BACKGROUND" != x ] && [ -f "$GRUB_BACKGROUND" ] \ > + fi > + > + if [ "x$GRUB_BACKGROUND" != x ] && [ -f "$GRUB_BACKGROUND" ] \ > && is_path_readable_by_grub "$GRUB_BACKGROUND"; then > gettext_printf "Found background: %s\n" "$GRUB_BACKGROUND" >&2 > case "$GRUB_BACKGROUND" in > > > > ___ > Grub-devel mailing list > Grub-devel@gnu.org > https://urldefense.com/v3/__https://lists.gnu.org/mailman/listinfo/grub- > devel__;!!ACWV5N9M2RV99hQ!PLGidopsW7qPo_wRq33QaUvrp0eXPF- > Qo0P_D2qJ8UL7kJdBXudmjssK0vWbpxFukgrhEoPThYgtOPZhaSDX$ ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v1 14/15] grub-install: install on EFI if forced
On 11/4/24 02:08, Neal Gompa wrote:
> On Thu, Oct 31, 2024 at 3:43 PM Leo Sandoval wrote:
>>
>> From: Marta Lewandowska
>>
>> UEFI Secure Boot requires signed grub binaries to work, so grub-
>> install should not be used. However, users who have Secure Boot
>> disabled and wish to use the command should not be prevented from
>> doing so if they invoke --force.
>>
>> fixes bz#1917213 / bz#2240994
>>
>> Signed-off-by: Marta Lewandowska
>> ---
>> util/grub-install.c | 38 +++---
>> 1 file changed, 19 insertions(+), 19 deletions(-)
>>
>> diff --git a/util/grub-install.c b/util/grub-install.c
>> index ee61b042b..b924c2d34 100644
>> --- a/util/grub-install.c
>> +++ b/util/grub-install.c
>> @@ -910,25 +910,6 @@ main (int argc, char *argv[])
>>
>>platform = grub_install_get_target (grub_install_source_directory);
>>
>> - switch (platform)
>> -{
>> -case GRUB_INSTALL_PLATFORM_ARM_EFI:
>> -case GRUB_INSTALL_PLATFORM_ARM64_EFI:
>> -case GRUB_INSTALL_PLATFORM_I386_EFI:
>> -case GRUB_INSTALL_PLATFORM_IA64_EFI:
>> -case GRUB_INSTALL_PLATFORM_LOONGARCH64_EFI:
>> -case GRUB_INSTALL_PLATFORM_RISCV32_EFI:
>> -case GRUB_INSTALL_PLATFORM_RISCV64_EFI:
>> -case GRUB_INSTALL_PLATFORM_X86_64_EFI:
>> - is_efi = 1;
>> - grub_util_error (_("this utility cannot be used for EFI platforms"
>> - " because it does not support UEFI Secure Boot"));
>> - break;
>> -default:
>> - is_efi = 0;
>> - break;
>> -}
>> -
>>{
>> char *platname = grub_install_get_platform_name (platform);
>> fprintf (stderr, _("Installing for %s platform.\n"), platname);
>> @@ -1045,6 +1026,22 @@ main (int argc, char *argv[])
>>
>>switch (platform)
>> {
>> +case GRUB_INSTALL_PLATFORM_ARM_EFI:
>> +case GRUB_INSTALL_PLATFORM_ARM64_EFI:
>> +case GRUB_INSTALL_PLATFORM_I386_EFI:
>> +case GRUB_INSTALL_PLATFORM_IA64_EFI:
>> +case GRUB_INSTALL_PLATFORM_LOONGARCH64_EFI:
>> +case GRUB_INSTALL_PLATFORM_RISCV32_EFI:
>> +case GRUB_INSTALL_PLATFORM_RISCV64_EFI:
>> +case GRUB_INSTALL_PLATFORM_X86_64_EFI:
>> + is_efi = 1;
>> + if (!force)
>> +grub_util_error (_("This utility should not be used for EFI
>> platforms"
>> + " because it does not support UEFI Secure Boot."
>> + " If you really wish to proceed, invoke the
>> --force"
>> + " option.\nMake sure Secure Boot is disabled
>> before"
>> + " proceeding"));
>> + break;
>> case GRUB_INSTALL_PLATFORM_I386_IEEE1275:
>> case GRUB_INSTALL_PLATFORM_POWERPC_IEEE1275:
>> #ifdef __linux__
>> @@ -1053,6 +1050,9 @@ main (int argc, char *argv[])
>> try_open ("/dev/nvram");
>> #endif
>>break;
>> + /* pacify warning. */
>> +case GRUB_INSTALL_PLATFORM_MAX:
>> + break;
>> default:
>>break;
>> }
>> --
>> 2.46.2
>>
>
> This should be merged with the patch that breaks grub-install for EFI
> and re-sent as one *new* patch. It's not okay to break it and then fix
> it in the same patch series, since we don't want broken functionality
> in a commit applied to git.
In addition, making mandatory the option --force when Secure Boot is not enabled
will need many changes of grub installation handling by many distributions,
admins and end users, and lead to a lot of systems unable to boot until done.
Why not instead, in case of Linux systems, check the Secure Boot status of the
firmware, for instance querying the NVRAM variable exposed as:
/sys/efi/efivars/SecureBoot-8be4df61-93ca-11d2-aa0d-00e098032b8c
and only if enabled, when running grub-install, display an error or warning?
Didier
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] kern/main: Fix cmdpath in root directory
On Mon, Nov 04, 2024 at 07:15:33PM GMT, Daniel Kiper wrote:
> On Fri, Nov 01, 2024 at 10:03:16AM +0800, Michael Chang wrote:
> > On Wed, Oct 30, 2024 at 05:12:48PM GMT, Daniel Kiper wrote:
> > > Adding Leo...
> > >
> > > On Tue, Oct 29, 2024 at 03:57:18PM +0800, Michael Chang via Grub-devel
> > > wrote:
> > > > The "cmdpath" environment variable is set at startup to the location
> > > > from which the grub image is loaded. It includes a device part and,
> > > > optionally, an absolute directory name if the grub image is booted as a
> > > > file in a local file-system directory, or in a remote server directory,
> > > > like TFTP.
> > > >
> > > > This entire process relies on firmware to provide the correct device
> > > > path of the booted image.
> > > >
> > > > We encountered an issue when the image is booted from the root
> > > > directory, where the absolute directory name "/" is discarded. This
> > > > makes it unclear whether the root path was missing in the firmware
> > > > provided device path or if it is simply the root directory. This
> > > > ambiguity can cause confusion in custom scripts, potentially causing
> > > > them to interpret firmware data incorrectly and trigger unintended
> > > > fallback measures.
> > > >
> > > > This patch fixes the problem by properly assigning the "fwpath" returned
> > > > by "grub_machine_get_bootlocation()" to "cmdpath". The fix is based on
> > > > the fact that fwpath is NULL if the firmware didn’t provide a path part
> > > > or an NUL character, "", if it represents the root directory. With this,
> > > > it becomes possible to clearly distinguish:
> > > >
> > > > - cmdpath=(hd0,1) - Either the image is booted from the first (raw)
> > > > partition, or the firmware failed to provide the path part.
> > > > - cmdpath=(hd0,1)/ - The image is booted from the root directory in the
> > > > first partition.
> > > >
> > > > As a side note, the fix is similar to [1], but without the renaming
> > > > part.
> > > >
> > > > [1] https://mail.gnu.org/archive/html/grub-devel/2024-10/msg00155.html
> > > >
> > > > Signed-off-by: Michael Chang
> > > > ---
> > > > grub-core/kern/main.c | 6 +-
> > > > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > > > index d29494d54..6645173c1 100644
> > > > --- a/grub-core/kern/main.c
> > > > +++ b/grub-core/kern/main.c
> > > > @@ -136,7 +136,11 @@ grub_set_prefix_and_root (void)
> > > > {
> > > >char *cmdpath;
> > > >
> > > > - cmdpath = grub_xasprintf ("(%s)%s", fwdevice, fwpath ? : "");
> > > > + if (fwpath && *fwpath == '\0')
> > > > + cmdpath = grub_xasprintf ("(%s)/", fwdevice);
> > > > + else
> > > > + cmdpath = grub_xasprintf ("(%s)%s", fwdevice, fwpath ? : "");
> > >
> > > Would not this below work?
> > >
> > > cmdpath = grub_xasprintf ("(%s)%s", fwdevice, (fwpath == NULL ||
> > > *fwpath == '\0') ? "/" : fwpath);
> >
> > It looks to me that this expression sets the path part of cmdpath to "/"
> > when fwpath == NULL. This is not desired, as it could mislead people
> > into thinking grub is booted from a root directory, when in fact it
> > might be a raw partition or disk.
>
> OK, then if/else requires a comment describing what is happening there.
OK. I will add it in next revision.
>
> > > And if yes does this (completely) replace the patch mentioned by you
> > > above?
> >
> > The mentioned patch is actually more complicated. I think it could be
> > broken down into three parts:
> >
> > 1. Fixing the boot from the root directory issue, as this patch does.
> > 2. Renaming cmdpath to fw_path.
> > 3. Using the new fw_path to look up grub.cfg.
> >
> > My concerns with respect to these three parts are:
> >
> > 1. Although it fixes the root directory issue, it also intentionally
> > ignores the case where fwpath is set to NULL. IMHO this is problematic
> > because booting from a raw partition or disk is valid, many setups such
> > as MBR, GPT BIOS Boot, and PPC PReP, are still actively used today.
>
> OK...
>
> > 2. Renaming the well-known cmdpath may break custom (third-party) scripts.
>
> This is not acceptable. So, cmdpath variable name cannot be changed.
> And I suggest to not change its name in downstream too.
>
> > 3. There is already a mechanism for looking up grub.cfg using the prefix
> > variable. If prefix is not set (e.g., when grub-mkimage is used without
> > specifying -p ..), its value is derived automatically from
> > fwdevice/fwpath. This makes the change seem redundant, as it’s already
> > covered.
>
> Which change are you referring to?
I was referring to this hunk in the patch "Add fw_path variable":
diff --git a/grub-core/normal/main.c b/grub-core/normal/main.c
index d3f53d93d..08f48c71d 100644
--- a/grub-core/normal/main.c
+++ b/grub-core/normal/main.c
@@ -339,7 +339,30 @@ grub_cmd_normal (struct grub_command *cmd __attribute__
((unused)),
/* Guess the config filename. It is necessary to make CONFIG static,
so that it won'
