Re: [PATCH 2/2] diskfilter: don't make a RAID array with more than 1024 disks
Daniel Axtens writes: > This is 'belt and braces' with the last fix: we end up trying to use > too much memory in situations like corrupted Linux software raid setups > purporting to usew a huge number of disks. Simply refuse to permit such > configurations. > > 1024 is a bit arbitrary, yes, and I feel a bit like I'm tempting fate > here, but I think 1024 disks in an array (that grub has to read to boot!) > should be enough for anyone. > > Signed-off-by: Daniel Axtens > --- > grub-core/disk/diskfilter.c | 7 +++ > 1 file changed, 7 insertions(+) > > diff --git a/grub-core/disk/diskfilter.c b/grub-core/disk/diskfilter.c > index 4ac50320ef4e..79c5f4db940a 100644 > --- a/grub-core/disk/diskfilter.c > +++ b/grub-core/disk/diskfilter.c > @@ -1046,6 +1046,13 @@ grub_diskfilter_make_raid (grub_size_t uuidlen, char > *uuid, int nmemb, >struct grub_diskfilter_pv *pv; >grub_err_t err; > > + /* We choose not to support more than 1024 disks */ > + if (nmemb > 1024) Ergh, nmemb is an int; I will do a v2 that also checks that it's greater than 0 (or 1, given that it's RAID? I will do some tests.) -- d ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] kern/efi/mm: Double the default heap size
Hector Martin writes:
> On 21/08/2022 21.35, Daniel Axtens wrote:
>> Hi Hector,
>>
>> Thanks for your patch and for taking the trouble to put it together.
>>
>>> GRUB is already running out of memory on Apple M1 systems, causing
>>> graphics init to fail, as of the latest Git changes. Since dynamic
>>> growing of the heap isn't done yet, double the default heap size for
>>> now.
>>
>> Huh, weird - those changes have landed in git, see commit 887f98f0db43
>> ("mm: Allow dynamically requesting additional memory regions") for the
>> overarching support and commit 1df2934822df ("kern/efi/mm: Implement
>> runtime addition of pages"). It's not done on PowerPC, but if you're in
>> EFI-land then it should complete.
>>
>> The only reason I can think of off the top of my head where you would be
>> having issues that your patch fixes is if we somehow need more memory to
>> even get to the point where we can ask firmware for more memory. I
>> suppose that's within the realm of possibility.
>
> Interesting. I missed the indirection through the function pointer...
> but either way, I do indeed have those commits in the broken tree that
> Arch Linux ARM started shipping yesterday (0c6c1aff2a, which isn't
> actually current master but it's from a couple weeks ago). The previous
> version was 2f4430cc0, which doesn't have it, so I wonder if there was
> actually a regression involved?
Hmm.
So I wonder if you're hitting an edge case which I tried to fix for
powerpc but apparently didn't fix for EFI (or, on reflection, in the
generic code where I should have tried to fix it). If you look at
kern/mm.c, we request `size` bytes from firmware:
case 1:
/* Request additional pages, contiguous */
count++;
if (grub_mm_add_region_fn != NULL &&
grub_mm_add_region_fn (size, GRUB_MM_ADD_REGION_CONSECUTIVE) ==
GRUB_ERR_NONE)
goto again;
but the problem is that we need some of those `size` bytes for grub mm
metadata. (We also don't respect `align`, eek.) The mm code doesn't
account for this (which it should, my bad) and the EFI code doesn't seem
to account for this either: (kern/efi/mm.c::grub_efi_mm_add_regions,
which is the function exposed as grub_mm_add_region_fn).
/* Allocate memory regions for GRUB's memory management. */
err = add_memory_regions (filtered_memory_map, desc_size,
filtered_memory_map_end,
BYTES_TO_PAGES (required_bytes),
flags);
Would you be able to try doing something like this instead?
modified grub-core/kern/efi/mm.c
@@ -621,7 +621,7 @@ grub_efi_mm_add_regions (grub_size_t required_bytes,
unsigned int flags)
/* Allocate memory regions for GRUB's memory management. */
err = add_memory_regions (filtered_memory_map, desc_size,
filtered_memory_map_end,
- BYTES_TO_PAGES (required_bytes),
+ BYTES_TO_PAGES (required_bytes) + 1,
flags);
if (err != GRUB_ERR_NONE)
return err;
> What I see is that GRUB briefly flashes an out of memory error and fails
> to set the graphics mode, then ends up in text mode. My best guess
> without digging further is that it fails to allocate a framebuffer or
> console text buffer (since these machines have higher resolution screens
> than most, this might not have come up elsewhere). But I don't see why
> that would have to happen before it's allowed?
>
Yeah that doesn't make much sense to me either.
Being a high dpi screen might make it more likely to line up with a page
size, thus hitting the edge case where we don't allocate metadata space?
That is one hypothesis.
Another would be whether the EFI implementation uses the page size grub
expects (which is 0x1000 - ISTR you're using 16k pages in the kernel?)
But I figure things would probably have broken in more interesting ways
if that was wrong...
>> I f my maths are right, this bumps up the initial allocation from 1M to
>> 2M.
>
> Correct.
>
>> I think your experience tends to disprove the hypothesis that we
>> could get away with a very small initial allocation (which was the
>> thinking when the initial dynamic allocation patch set went in), so I'm
>> wondering if we should take this opportunity to allocate 16M or 32M or
>> something. My powerpc proposal kept the initial allocation at 32MB, I
>> think that's probably sane for EFI too?
>
> I think that makes sense.
(I still think this is worth doing even if it turns out that you just got
unlucky and hit the edge case.)
Kind regards,
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v4 5/5] Don't display a uefi-firmware entry if it's not supported
Glenn Washburn writes:
> On Thu, 18 Aug 2022 13:50:13 -0400
> Robbie Harwood wrote:
>
>> Add a new --is-supported option to commands/efi/efifwsetup and
>> conditionalize display on it.
>>
>> Signed-off-by: Robbie Harwood
>> ---
>> grub-core/commands/efi/efifwsetup.c | 3 +++
>> util/grub.d/30_uefi-firmware.in | 2 +-
>> 2 files changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/grub-core/commands/efi/efifwsetup.c
>> b/grub-core/commands/efi/efifwsetup.c
>> index cb4b6ff18c..53d23deea7 100644
>> --- a/grub-core/commands/efi/efifwsetup.c
>> +++ b/grub-core/commands/efi/efifwsetup.c
>> @@ -40,6 +40,9 @@ grub_cmd_fwsetup (grub_command_t cmd __attribute__
>> ((unused)),
>>grub_size_t oi_size;
>>static grub_efi_guid_t global = GRUB_EFI_GLOBAL_VARIABLE_GUID;
>>
>> + if (argc >= 1 && grub_strcmp(args[0], "--is-supported") == 0)
>> +return !efifwsetup_is_supported ();
>> +
>>if (!efifwsetup_is_supported ())
>>return grub_error (GRUB_ERR_INVALID_COMMAND,
>> N_("reboot to firmware setup is not supported by
>> the current firmware"));
>> diff --git a/util/grub.d/30_uefi-firmware.in
>> b/util/grub.d/30_uefi-firmware.in
>> index b6041b55e2..78aef67d78 100644
>> --- a/util/grub.d/30_uefi-firmware.in
>> +++ b/util/grub.d/30_uefi-firmware.in
>> @@ -31,7 +31,7 @@ LABEL="UEFI Firmware Settings"
>> gettext_printf "Adding boot menu entry for UEFI Firmware Settings ...\n" >&2
>>
>> cat << EOF
>> -if [ "\$grub_platform" = "efi" ]; then
>> +if [ "\$grub_platform" = "efi" ] && fwsetup --is-supported; then
>
> I don't believe that grub currently supported the && syntax. I just
> tested and I'm getting syntax errors.
>
> Untested, but I believe the following should work:
>
> fwsetup --is-supported
> if [ "\$grub_platform" = "efi" -a "\$?" = "0" ]; then
Good catch, thanks.
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: [PATCH v2] grub-core/kern/corecmd: Quote variable values when displayed by the set command
On Sat, 20 Aug 2022 01:07:35 +0200
Daniel Kiper wrote:
> On Fri, Aug 19, 2022 at 05:38:24PM -0500, Glenn Washburn wrote:
> > Variable values may contain spaces at the end or newlines. However, when
> > displayed without quotes this is not obvious and can lead to confusion as
> > to the actual contents of variables. Also for some variables grub_env_get()
> > returns a NULL pointer instead of a pointer to an empty string and
> > previously would be printed as 'var=(null)'. Now such variables will be
> > displayed as 'var=""'.
>
> Better but...
>
> > Signed-off-by: Glenn Washburn
> > ---
> > grub-core/kern/corecmd.c | 5 -
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/grub-core/kern/corecmd.c b/grub-core/kern/corecmd.c
> > index fc54f43f2..10b595d6b 100644
> > --- a/grub-core/kern/corecmd.c
> > +++ b/grub-core/kern/corecmd.c
> > @@ -40,7 +40,10 @@ grub_core_cmd_set (struct grub_command *cmd
> > __attribute__ ((unused)),
> > {
> >struct grub_env_var *env;
> >FOR_SORTED_ENV (env)
> > - grub_printf ("%s=%s\n", env->name, grub_env_get (env->name));
> > + {
> > + val = grub_env_get (env->name);
> > + grub_printf ("%s=\"%s\"\n", env->name, val ? val : "");
>
> Maybe I am overzealous but what will happen if value contains "$"
> character, e.g. "$var", and somebody wants copy-pasta this value with
> "" around? I think '' instead of "" would be better here. IIRC '' works
> in GRUB shell like it works in regular shell.
Sure, single quotes seem good.
>
> Additionally, what if the value contains '"' characters? Should we
> escape them?
Yes, this would be ideal. I don't really want to be the one to implement
this though. The use case that I care about it seeing when variables
have whitespace at the end of the value. This is more for debugging
GRUB script that is _not_ created with the intention of making the
output of set trick the user. Also I don't have grub script in
variables (though this could be reasonable with someone using exec), so
its not an issue for me.
>
> And a nit... I prefer s/val ?/(val != NULL) ?/.
Yep, of course.
Glenn
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Add some randomness to TCP source port selection.
Anyone willing to implement this in a better way? Thank you, Robert LeBlanc Robert LeBlanc PGP Fingerprint 79A2 9CA4 6CC4 45DD A904 C70E E654 3BB2 FA62 B9F1 Robert LeBlanc PGP Fingerprint 79A2 9CA4 6CC4 45DD A904 C70E E654 3BB2 FA62 B9F1 On Tue, Jun 14, 2022 at 7:19 AM Robert LeBlanc wrote: > > Thanks. > > I had trouble using the clock to seed the random number generator due to some > dependency issues. I'm not strong enough in C to figure that out with the > limited GRUB libraries (since standard libraries are not used), so I did what > I could to show the intended behavior. Please feel free to submit a much > better patch as I don't have the expertise to do so. > > Thank you, > Robert LeBlanc > > Sent from a mobile device, please excuse any typos. > > On Mon, Jun 6, 2022, 11:27 AM Vladimir 'phcoder' Serbinenko > wrote: >> >> >> >> Le lun. 6 juin 2022, 19:25, Vladimir 'phcoder' Serbinenko >> a écrit : >>> >>> 256 is a bad modulo. A prime would be a much better one for those purposes. >>> Also get_time_ms counts up from arbitrary point in time, often boot. I >>> suggest using some combination of etc >> >> RTC, not etc >>> >>> >>> and get_time to seed an LFSR algorithm >>> >>> Le lun. 6 juin 2022, 18:37, Robert LeBlanc a écrit : GRUB uses a static source TCP port and increments for each new connection. When rapidly restarting GRUB this can cause issues with some firewalls that suspect that a reply attack is happening. In addition GRUB does not ACK the last FIN,ACK when booting the kernel and initrd from HTTP for example. This cause the remote HTTP server to keep the TCP session in TIME_WAIT and reject new connections from the same port combination when restarted quickly. This helps to work around both problems by shifting the source port by a small amount based on time. The missing final ACK should also be addressed, but I'm not sure how to resolve that. Signed-off-by: Robert LeBlanc --- grub-core/net/tcp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/grub-core/net/tcp.c b/grub-core/net/tcp.c index 93dee0caa..2eefd3168 100644 --- a/grub-core/net/tcp.c +++ b/grub-core/net/tcp.c @@ -569,7 +569,7 @@ grub_net_tcp_open (char *server, struct grub_net_network_level_interface *inf; grub_net_network_level_address_t gateway; grub_net_tcp_socket_t socket; - static grub_uint16_t in_port = 21550; + grub_uint16_t in_port = 21550 + grub_get_time_ms () % 256; struct grub_net_buff *nb; struct tcphdr *tcph; int i; @@ -603,7 +603,7 @@ grub_net_tcp_open (char *server, socket->inf = inf; socket->out_nla = addr; socket->ll_target_addr = ll_target_addr; - socket->in_port = in_port++; + socket->in_port = in_port; socket->recv_hook = recv_hook; socket->error_hook = error_hook; socket->fin_hook = fin_hook; -- 2.35.1 ___ 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 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH] grub-shell: Put all generated files into working dir and use better file names
When running tests there are many invocations of grub-shell, and because the
output files are all random names in the same tmp directory, it becomes more
work to figure out which files went with which grub-shell invocations. So
all generated files from one invocation of grub-shell are put into a
randomly named directory, so as not to collide with other grub-shell
invocations. And now that the generated files can be put in a location where
they will not get stepped on, and they can be named sensible names.
Signed-off-by: Glenn Washburn
---
tests/util/grub-shell.in | 22 +-
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/tests/util/grub-shell.in b/tests/util/grub-shell.in
index ce431757c..6cb72403e 100644
--- a/tests/util/grub-shell.in
+++ b/tests/util/grub-shell.in
@@ -70,6 +70,8 @@ exec_show_error () {
fi
}
+work_directory=${WORKDIR:-`mktemp -d "${TMPDIR:-/tmp}/grub-shell.XX"`}
|| exit 1
+
. "${builddir}/grub-core/modinfo.sh"
qemuopts="${GRUB_QEMU_OPTS}"
serial_port=com0
@@ -79,7 +81,7 @@ pseries=n
disk="hda "
case "${grub_modinfo_target_cpu}-${grub_modinfo_platform}" in
*-emu)
- device_map=`mktemp "${TMPDIR:-/tmp}/tmp.XX"` || exit 1
+ device_map="$work_directory/device.map"
boot=emu
console=console
disk=0
@@ -313,14 +315,14 @@ for option in "$@"; do
done
if [ "x${source}" = x ] ; then
-tmpfile=`mktemp "${TMPDIR:-/tmp}/tmp.XX"` || exit 1
+tmpfile="$work_directory/testcase.cfg"
while read REPLY; do
echo "$REPLY" >> ${tmpfile}
done
source=${tmpfile}
fi
-cfgfile=`mktemp "${TMPDIR:-/tmp}/tmp.XX"` || exit 1
+cfgfile="$work_directory/grub.cfg"
cat <${cfgfile}
grubshell=yes
enable_progress_indicator=0
@@ -354,7 +356,8 @@ if [ $trim = 1 ]; then
echo "echo $trim_head" >>${cfgfile}
fi
-rom_directory=`mktemp -d "${TMPDIR:-/tmp}/tmp.XX"` || exit 1
+rom_directory="$work_directory/rom_directory"
+mkdir -p "$rom_directory"
for mod in ${modules}
do
@@ -375,7 +378,7 @@ test -z "$debug" || echo "GRUB script: ${cfgfile}" >&2
test -z "$debug" || echo "GRUB testcase script: ${tmpfile}" >&2
test -z "$debug" || echo "Boot device: ${boot}" >&2
-isofile=`mktemp "${TMPDIR:-/tmp}/tmp.XX"` || exit 1
+isofile="$work_directory/grub.iso"
test -z "$debug" || echo "GRUB ISO file: ${isofile}" >&2
test -z "$debug" || echo "GRUB ROM directory: ${rom_directory}" >&2
@@ -451,7 +454,7 @@ if [ x$boot = xmips_qemu ]; then
fi
if [ x$boot = xcoreboot ]; then
-imgfile=`mktemp "${TMPDIR:-/tmp}/tmp.XX"` || exit 1
+imgfile="$work_directory/coreboot.img"
cp "${GRUB_COREBOOT_ROM}" "${imgfile}"
"${GRUB_CBFSTOOL}" "${imgfile}" add-payload -f
"${rom_directory}/coreboot.elf" -n fallback/payload
bootdev="-bios ${imgfile}"
@@ -494,14 +497,15 @@ copy_extra_files() {
}
if [ x$boot = xnet ]; then
-netdir=`mktemp -d "${TMPDIR:-/tmp}/tmp.XX"` || exit 1
+netdir="$work_directory/netdir"
+mkdir -p "$netdir"
pkgdatadir="${builddir}" "${builddir}/grub-mknetdir"
"--grub-mkimage=${builddir}/grub-mkimage" "--directory=${builddir}/grub-core"
"--net-directory=$netdir" ${mkrescue_args} > /dev/null
cp "${cfgfile}" "$netdir/boot/grub/grub.cfg"
cp "${source}" "$netdir/boot/grub/testcase.cfg"
[ -z "$files" ] || copy_extra_files "$netdir" $files
timeout -s KILL $timeout "${qemu}" ${qemuopts} ${serial_null} -serial
file:/dev/stdout -boot n -net
"user,tftp=$netdir,bootfile=/boot/grub/${grub_modinfo_target_cpu}-${grub_modinfo_platform}/core.$netbootext"
-net nic | cat | tr -d "\r" | do_trim
elif [ x$boot = xemu ]; then
-rootdir="$(mktemp -d "${TMPDIR:-/tmp}/tmp.XX")"
+rootdir="$work_directory/rootdir"
grubdir="$rootdir/boot/grub"
mkdir -p "$grubdir/fonts"
mkdir -p "$grubdir/themes"
@@ -516,7 +520,7 @@ elif [ x$boot = xemu ]; then
cp "${cfgfile}" "$grubdir/grub.cfg"
cp "${source}" "$grubdir/testcase.cfg"
[ -z "$files" ] || copy_extra_files "$rootdir" $files
-roottar="$(mktemp "${TMPDIR:-/tmp}/tmp.XX")"
+roottar="$work_directory/root.tar"
(cd "$rootdir"; tar cf "$roottar" .)
"${builddir}/grub-core/grub-emu" -m "$device_map" --memdisk "$roottar" -r
memdisk -d "/boot/grub" | tr -d "\r" | do_trim
test -n "$debug" || rm -rf "$rootdir"
--
2.34.1
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v6 1/1] plainmount: Support plain encryption mode
I noticed that you've not been including Daniel Kiper on these patches.
I don't expect him to look at these until I'm satisfied with the patch,
but its always good practice to include him. So please do on the next
version of this patch.
On Sun, 31 Jul 2022 17:32:50 +
Maxim Fomin wrote:
> From 683357e227467c05272facc7da534a82becc9d8a Mon Sep 17 00:00:00 2001
> From: Maxim Fomin
> Date: Sun, 31 Jul 2022 14:06:42 +0100
> Subject: [PATCH v6 1/1] plainmount: Support plain encryption mode
>
> This patch adds support for plain encryption mode (plain dm-crypt) via
> new module/command named 'plainmount'.
>
> Signed-off-by: Maxim Fomin
> ---
> docs/grub.texi | 80 +++
> grub-core/Makefile.core.def | 5 +
> grub-core/disk/plainmount.c | 450
> 3 files changed, 535 insertions(+)
> create mode 100644 grub-core/disk/plainmount.c
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index af119dea3..22c73580c 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -4265,6 +4265,7 @@ you forget a command, you can run the command
> @command{help}
> * parttool::Modify partition table entries
> * password::Set a clear-text password
> * password_pbkdf2:: Set a hashed password
> +* plainmount:: Open device encrypted in plain mode
> * play::Play a tune
> * probe:: Retrieve device info
> * rdmsr:: Read values from model-specific registers
> @@ -4552,6 +4553,14 @@ function is supported, as Argon2 is not yet supported.
>
> Also, note that, unlike filesystem UUIDs, UUIDs for encrypted devices must be
> specified without dash separators.
> +
> +Successfully decrypted disks are named as (cryptoX) and have increasing
> numeration
> +suffix for each new decrypted disk. If the encrypted disk hosts some higher
> level
> +of abstraction (like LVM2 or MDRAID) it will be created under a separate
> device
> +namespace in addition to the cryptodisk namespace.
> +
> +Support for plain encryption mode (plain dm-crypt) is provided via separate
> +@command{@pxref{plainmount}} command.
> @end deffn
>
> @node cutmem
> @@ -5060,6 +5069,77 @@ to generate password hashes. @xref{Security}.
> @end deffn
>
>
> +@node plainmount
> +@subsection plainmount
> +
> +@deffn Command plainmount device @option{-c} cipher @option{-s} key size
> [@option{-h} hash]
> +[@option{-S} sector size] [@option{-p} password] [@option{-u} uuid]
> +[[@option{-d} keyfile] [@option{-O} keyfile offset]]
> +
> +
> +Setup access to the encrypted device in plain mode. Offset of the encrypted
> +data at the device is specified in terms of 512 byte sectors with the
> blocklist
> +syntax and loopback device. The following example shows how to specify 1MiB
> +offset:
> +
> +@example
> +loopback node (hd0,gpt1)2048+
> +plainmount node
> +@end example
> +
> +The @command{plainmount} command can be used to open LUKS encrypted volume
> +if its master key and parameters (key size, cipher, offset, etc) are known.
> +
> +There are two ways to specify password: a keyfile and a secret passphrase.
s/password/a password/
> +Keyfile path parameter has higher priority than secret passphrase and is
s/Keyfile/The keyfile/
s/secret passphrase/the secret passphrase parameter/
> +specified with the option @option{-d}. Password data obtained from keyfiles
> +is not hashed and is used directly as a cipher key. Optional offset of
> password
s/Optional/An optional/
> +data in the keyfile can be specified with the option @option{-O} or directly
> +with the option @option{-d} and GRUB blocklist syntax. The following example
> +shows both methods to specify password data in the keyfile at offset 1MiB:
> +
> +@example
> +plainmount -d (hd0,gpt1)2048+
> +plainmount -d (hd0,gpt1) -O 1048576
I don't think this will work. I think it has to be something like:
plainmount -d (hd0,gpt1)+ -O 1048576
> +@end example
> +
> +If no keyfile is specified then the password is set to data specified by
s/data/the string/
> +option @option{-p} or is requested interactively from the console. In both
> +cases the provided password is hashed with the algorithm specified by the
> +option @option{-h}. This option is mandatory if no keyfile is specified, but
> +it can be set to @code{plain} which means that no hashing is done and such
I was mistaken in my last suggestion to use @code here, instead @samp
should be used because this is sample input.
> +password is used directly as a key.
> +
> +Cipher @option{-c} and keysize @option{-s} options specify the cipher
> +algorithm and the key size respectively and are mandatory options. Cipher
> +must be specified with the mode separated by a dash (for example,
> +'aes-xts-plain64'). Key size option @option{-s} is the key size of the
> cipher,
s/'aes-xts-plain64'/@samp{aes-xts-plain64}/
s/cipher/cipher in bits/
> +not to be confused with the offset o
