Re: [PATCH] diskfilter: don't make a RAID array with more than 1024 disks
On Wed, Oct 19, 2022 at 08:23:22PM +1100, Daniel Axtens wrote:
> This is 'belt and braces' with commit 12e20a6a695f ("disk/diskfilter:
> Check calloc() result for NULL"): we end up trying to use too much memory
> in situations like corrupted Linux software raid setups purporting to
> use 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
Reviewed-by: Daniel Kiper
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v4 2/2] mm: Better handling of adding new regions
On Sat, Oct 15, 2022 at 10:15:12PM +0800, Zhang Boyang wrote:
> Previously, there are two problems of the handling of adding new
Please drop "Previously". It is not needed...
> regions. First, it doesn't take region management cost into account.
> Adding a memory area of `x` bytes will result in a heap region of less
> than `x` bytes avaliable. Thus, the new region might not adequate for
> current allocation request. Second, it doesn't pre-allocate a big region
> for small allocations, so it might be slow for many small allocations.
>
> This patch introduces GRUB_MM_MAX_COST to address the cost of region
> management. The value of this constant should make
> `grub_mm_init_region (addr, size + GRUB_MM_MAX_COST)` initialize a
> region of at least `size` free bytes available. The size of heap growth
> is now adjusted according this value before calling
> grub_mm_add_region_fn ().
This should be separate patch...
> This patch also introduces GRUB_MM_HEAP_GROW to address the minimal heap
> growth granularity. When a small allocation encounters heap space
> exhaustion, the size of heap growth is now expended to this value, and a
> new region of bigger size will be added. Thus subsequent allocations can
> use the remaining space of that new region.
... and this should be the second one, to be precise third patch in the
series...
> Signed-off-by: Zhang Boyang
> ---
> grub-core/kern/mm.c | 18 +++---
> include/grub/mm_private.h | 12
> 2 files changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/grub-core/kern/mm.c b/grub-core/kern/mm.c
> index ae2279133..fe630f061 100644
> --- a/grub-core/kern/mm.c
> +++ b/grub-core/kern/mm.c
> @@ -410,7 +410,9 @@ grub_memalign (grub_size_t align, grub_size_t size)
> {
>grub_mm_region_t r;
>grub_size_t n = ((size + GRUB_MM_ALIGN - 1) >> GRUB_MM_ALIGN_LOG2) + 1;
> + grub_size_t guarantee;
>int count = 0;
> + grub_size_t grow;
>
>if (!grub_mm_base)
> goto fail;
> @@ -418,10 +420,13 @@ grub_memalign (grub_size_t align, grub_size_t size)
>if (size > ~(grub_size_t) align)
> goto fail;
>
> + /* Allocation can always success if max continuous free chunk >= guarantee
> */
> + guarantee = size + align;
> +
>/* We currently assume at least a 32-bit grub_size_t,
> so limiting allocations to - 1MiB
> in name of sanity is beneficial. */
> - if ((size + align) > ~(grub_size_t) 0x10)
> + if (guarantee > ~(grub_size_t) 0x10)
> goto fail;
>
>align = (align >> GRUB_MM_ALIGN_LOG2);
> @@ -443,11 +448,18 @@ grub_memalign (grub_size_t align, grub_size_t size)
>switch (count)
> {
> case 0:
> + /* Calculate optimal size of heap growth. */
> + grow = grub_max(guarantee, GRUB_MM_HEAP_GROW);
Missing space before "(".
> +
> + /* Adjust heap growth to address the cost of region management. */
> + if (grub_add(grow, GRUB_MM_MAX_COST, &grow))
Ditto...
> + goto fail;
> +
>/* 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)
> + grub_mm_add_region_fn (grow, GRUB_MM_ADD_REGION_CONSECUTIVE) ==
> GRUB_ERR_NONE)
> goto again;
>
>/* fallthrough */
> @@ -462,7 +474,7 @@ grub_memalign (grub_size_t align, grub_size_t size)
> * Try again even if this fails, in case it was able to partially
> * satisfy the request
> */
> - grub_mm_add_region_fn (size, GRUB_MM_ADD_REGION_NONE);
> + grub_mm_add_region_fn (grow, GRUB_MM_ADD_REGION_NONE);
>goto again;
> }
>
> diff --git a/include/grub/mm_private.h b/include/grub/mm_private.h
> index 96c2d816b..a01ba2507 100644
> --- a/include/grub/mm_private.h
> +++ b/include/grub/mm_private.h
> @@ -95,6 +95,18 @@ typedef struct grub_mm_region
> }
> *grub_mm_region_t;
>
> +/*
> + * Set an upper bound of management cost of each region,
> + * with sizeof(struct grub_mm_region), sizeof(struct grub_mm_header) and
> + * any possible alignment or padding taken into account.
OK...
> + * The value should make `grub_mm_init_region (addr, size +
> GRUB_MM_MAX_COST)`
> + * initialize a region of at least `size` free bytes available.
> + */
> +#define GRUB_MM_MAX_COST 0x1000
... but why do not you use sizeof() here then?
> +
> +/* Minimal heap growth granularity, when existing heap space is exhausted. */
> +#define GRUB_MM_HEAP_GROW 0x10
It would be nice if you shortly explain in the comment before why you
come up with this value.
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [RFC PATCH v3 2/2] mm: Separate different types of allocations into different regions
On Sat, Oct 15, 2022 at 11:00:35AM +0200, Patrick Steinhardt wrote: > On Thu, Oct 13, 2022 at 09:29:19AM +0800, Zhang Boyang wrote: > > This patch add type infomation to heap regions. Currently there are four > > types: GRUB_MM_SIZE_SMALL, GRUB_MM_SIZE_LARGE, GRUB_MM_CLASS_DATA, > > GRUB_MM_CLASS_MODULE. Each heap region can have its own mask of > > acceptable memory allocation types. > > > > The GRUB_MM_SIZE_* types denotes the relative size of an allocation. > > Allocations lesser than GRUB_MM_GROW_SMALL belong to GRUB_MM_SIZE_SMALL, > > otherwise they belong to GRUB_MM_SIZE_LARGE. Thus small and large chunks > > can be separated into different heaps. This might help reduce heap > > fragmentation. > > > > The GRUB_MM_CLASS_* types denotes whether an allocation is a module > > segment. Module segments are allocated with GRUB_MM_CLASS_MODULE flag > > set. Plain data chunks (and module metadata) are allocated with > > GRUB_MM_CLASS_DATA flag set. Thus module segments and plain data chunks > > can be separated into different heaps. > > > > On platforms with dynamic heap growth support, when heap space is > > exhausted, a new region will be allocated from firmware. The new region > > has same type flags as the allocation request itself. The size of new > > region will be (size + align + GRUB_MM_MAX_COST) for GRUB_MM_SIZE_LARGE > > requests, and (GRUB_MM_GROW_SMALL + GRUB_MM_MAX_COST) for > > GRUB_MM_SIZE_SMALL requests, with GRUB_MM_MAX_COST denotes the extra > > region management cost. > > > > On platforms without dynamic heap growth support, default heap will be > > created with GRUB_MM_TYPE_ALL, so any type of allocation can be > > fulfilled by default heap. > > I wonder whether we can again split this patch up into at least two > parts: the first one that starts to round memory allocations, and then > the second one to introduce memory types. The former would likely not be > all that controversial. So by splitting this patch up we can already > land the two patches to reorder disk cache invalidation and round > requests, both of which should provide immediate benefit to users. > > The introduction of memory request types can then be discussed > separately. Yeah, I agree. Though first of all I would prefer fully understand all pros and cons of memory request types in the bootloader. At this point I would be more than happy if we simplify memory allocator/relocator than add more futures to it. I think the bootloader does not need complicated allocators covering all corner cases due to its quite short live time. I prefer something which is simple and reliable. And probably fast... Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] multiboot: fix memory leak
On Sat, Oct 15, 2022 at 06:41:12PM +0800, t.feng via Grub-devel wrote:
> follow up the commit:
> eb33e61b31902a5493468895aaf83fa0b4f5f59d
>
> it seems that old commit can not fix memory leak completely.
The commit message should look more or less like that:
The commit eb33e61b3 (multiboot: fix memory leak) did not fix all
issues. Fix all of them right now.
Fixes: eb33e61b3 (multiboot: fix memory leak)
Signed-off-by: t.feng
> ---
> grub-core/loader/multiboot_elfxx.c | 17 +
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/grub-core/loader/multiboot_elfxx.c
> b/grub-core/loader/multiboot_elfxx.c
> index b76451f73..056992d80 100644
> --- a/grub-core/loader/multiboot_elfxx.c
> +++ b/grub-core/loader/multiboot_elfxx.c
> @@ -246,10 +246,7 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t
> *mld)
> return grub_errno;
>
>if (grub_file_seek (mld->file, ehdr->e_shoff) == (grub_off_t) -1)
> - {
> - grub_free (shdr);
> - return grub_errno;
> - }
> + goto fail;
>
>if (grub_file_read (mld->file, shdr, shnum * ehdr->e_shentsize)
>!= (grub_ssize_t) shnum * ehdr->e_shentsize)
> @@ -257,7 +254,7 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
> if (!grub_errno)
> grub_error (GRUB_ERR_FILE_READ_ERROR, N_("premature end of file
> %s"),
> mld->filename);
> - return grub_errno;
> + goto fail;
> }
>
>for (shdrptr = shdr, i = 0; i < shnum;
There is an additional leak in line 271. Please fix it too and double
check there are no other ones.
> @@ -286,14 +283,15 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t
> *mld)
> mld->avoid_efi_boot_services);
> if (err != GRUB_ERR_NONE)
> {
> + grub_errno = err;
> grub_dprintf ("multiboot_loader", "Error loading shdr %d\n", i);
> - return err;
> + goto fail;
> }
> src = get_virtual_current_address (ch);
> target = get_physical_target_address (ch);
>
> if (grub_file_seek (mld->file, sh->sh_offset) == (grub_off_t) -1)
> - return grub_errno;
> + goto fail;
>
>if (grub_file_read (mld->file, src, sh->sh_size)
>!= (grub_ssize_t) sh->sh_size)
> @@ -301,16 +299,19 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t
> *mld)
> if (!grub_errno)
> grub_error (GRUB_ERR_FILE_READ_ERROR, N_("premature end of file
> %s"),
> mld->filename);
> - return grub_errno;
> + goto fail;
> }
> sh->sh_addr = target;
> }
>GRUB_MULTIBOOT (add_elfsyms) (shnum, ehdr->e_shentsize,
> shstrndx, shdr);
> + return GRUB_ERR_NONE;
> }
>
> #undef phdr
>
> +fail:
Missing space before "fail:" label.
> + grub_free (shdr);
>return grub_errno;
> }
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v4] templates: introduce GRUB_TOP_LEVEL_* vars
Tue, 18 Oct 2022 21:53:32 -0700 Denton Liu : > On Tue, Oct 18, 2022 at 04:18:21PM +0200, Olaf Hering wrote: > > Maybe the patch description lacks a specific example how the proposed > > change is supposed to be used in your environment. > My patch description says: > Introduce the GRUB_TOP_LEVEL, GRUB_TOP_LEVEL_XEN and > GRUB_TOP_LEVEL_OS_PROBER variables to allow users to specify the > top-level entry. > and I'm not quite sure how to make it more clear other than, perhaps, > explaining what the top-level entry means. After reading the patch again, the newly added documentation states: "This option should be a path to a kernel image." I think it needs to be more specific: is it expecting an absolute path, or just the basename of the desired image? Olaf pgpWLJKooUmYC.pgp Description: Digitale Signatur von OpenPGP ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v5 1/1] Add support for grub-emu to kexec Linux menu entries
On Wed, Oct 19, 2022 at 02:50:00PM -0400, Robbie Harwood wrote:
> From: Raymund Will
>
> The GRUB emulator is used as a debugging utility but it could also be
> used as a user-space bootloader if there is support to boot an operating
> system.
>
> The Linux kernel is already able to (re)boot another kernel via the
> kexec boot mechanism. So the grub-emu tool could rely on this feature
> and have linux and initrd commands that are used to pass a kernel,
> initramfs image and command line parameters to kexec for booting a
> selected menu entry.
>
> By default the systemctl kexec option is used so systemd can shutdown
> all of the running services before doing a reboot using kexec. But if
> this is not present, it can fall back to executing the kexec user-space
> tool directly. The ability to force a kexec-reboot when systemctl kexec
> fails must only be used in controlled environments to avoid possible
> filesystem corruption and data loss.
>
> Signed-off-by: Raymund Will
> Signed-off-by: John Jolly
> Signed-off-by: Javier Martinez Canillas
> [rharwood: documentation, code style]
> Signed-off-by: Robbie Harwood
> ---
> docs/grub.texi | 28 --
> grub-core/Makefile.am| 1 +
> grub-core/Makefile.core.def | 2 +-
> grub-core/kern/emu/main.c| 4 +
> grub-core/kern/emu/misc.c| 18 +++-
> grub-core/loader/emu/linux.c | 183 +++
> include/grub/emu/exec.h | 4 +-
> include/grub/emu/hostfile.h | 3 +-
> include/grub/emu/misc.h | 3 +
> 9 files changed, 233 insertions(+), 13 deletions(-)
> create mode 100644 grub-core/loader/emu/linux.c
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index af119dea3b..1463e62e6b 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -1,4 +1,4 @@
> -\input texinfo
> +i\input texinfo
> @c -*-texinfo-*-
> @c %**start of header
> @setfilename grub.info
> @@ -923,17 +923,17 @@ magic.
> @node General boot methods
> @section How to boot operating systems
>
> -GRUB has two distinct boot methods. One of the two is to load an
> -operating system directly, and the other is to chain-load another boot
> -loader which then will load an operating system actually. Generally
> -speaking, the former is more desirable, because you don't need to
> -install or maintain other boot loaders and GRUB is flexible enough to
> -load an operating system from an arbitrary disk/partition. However,
> -the latter is sometimes required, since GRUB doesn't support all the
> -existing operating systems natively.
> +GRUB has three distinct boot methods: loading an operating system
> +directly, using kexec from userspace, and chainloading another
> +bootloader. Generally speaking, the first two are more desirable
> +because you don't need to install or maintain other boot loaders and
> +GRUB is flexible enough to load an operating system from an arbitrary
> +disk/partition. However, chainloading is sometimes required, as GRUB
> +doesn't support all existing operating systems natively.
>
> @menu
> * Loading an operating system directly::
> +* Kexec::
> * Chain-loading::
> @end menu
>
> @@ -959,6 +959,16 @@ use more complicated instructions. @xref{DOS/Windows},
> for more
> information.
>
>
> +@node Kexec
> +@subsection Kexec with grub2-emu
> +
> +grub2 can be run in userspace by invoking the grub2-emu tool. It will
Please be consistent, s/grub2/GRUB/ if you use "GRUB" above...
> +read all configuration scripts as if booting directly (see @xref{Loading
> +an operating system directly}). With the @code{--kexec} flag, and
> +kexec(8) support from the operating system, the @command{linux} command
> +will directly boot the target image.
> +
> +
> @node Chain-loading
> @subsection Chain-loading an OS
>
> diff --git a/grub-core/Makefile.am b/grub-core/Makefile.am
> index ee88e44e97..80e7a83edf 100644
> --- a/grub-core/Makefile.am
> +++ b/grub-core/Makefile.am
> @@ -307,6 +307,7 @@ KERNEL_HEADER_FILES +=
> $(top_srcdir)/include/grub/emu/net.h
> KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/emu/hostdisk.h
> KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/emu/hostfile.h
> KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/extcmd.h
> +KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/emu/exec.h
> if COND_GRUB_EMU_SDL
> KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/sdl.h
> endif
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 7159948721..5350408601 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -1816,9 +1816,9 @@ module = {
>arm64 = loader/arm64/linux.c;
>riscv32 = loader/riscv/linux.c;
>riscv64 = loader/riscv/linux.c;
> + emu = loader/emu/linux.c;
>common = loader/linux.c;
>common = lib/cmdline.c;
> - enable = noemu;
> };
>
> module = {
> diff --git a/grub-core/kern/emu/main.c b/grub-core/kern/emu/main.c
> index 44e087e988..855b11c3de 100644
> --- a/grub-core/kern/emu/main.c
> +++ b/grub-core/kern/emu/
Re: Adding --set to videoinfo to retrieve current video resolution
Adding Paul, Vladimir, and Zhang...
On Wed, Oct 12, 2022 at 03:36:13PM +0200, Markus Scholz wrote:
> Dear Grub Developers,
>
> recently we faced the problem the author in this old thread faced:
> - https://lists.gnu.org/archive/html/grub-devel/2012-10/msg9.html
>
> Ie. our theme wouldn't display properly due to different screen resolutions.
> Hence, I wanted to change specifically the font size of our theme given the
> current resolution.
> While the videoinfo module provides info about the current resolution I
> wasn't able to
> discover any way to get the info from the booted grub.
>
> Following the aforementioned thread, I therefore patched the videoinfo mod
> to add the ability
> of a -set switch just like commands like "smbinfo" or "search" have.
>
> Ie. the documentation of the videoinfo call could change as follows:
>
> Command: videoinfo [--set var | [WxH]xD]
>
> Retrieve available video modes. If --set is given, assign the currently
> active resolution to var.
> If --set is not provided the available video modes are listed.
> If resolution is given, show only matching modes.
>
> Attached is my patch proposal for the module; I probably made a lot mistakes
> wrt.
> coding style and guidelines - please bear with me.
>
> I would be grateful regarding feedback what I could do / how I could improve
> the patch to make it part of grub? If it possible at all with this approach?
I think fix for similar problem has been proposed here [1] and
I commented it here [2]. Sadly it ended in limbo. Could you work
with Zhang on this issue?
Daniel
[1] https://lists.gnu.org/archive/html/grub-devel/2022-06/msg00143.html
[2] https://lists.gnu.org/archive/html/grub-devel/2022-07/msg00027.html
> Thank you for your reply & best,
> Markus
>
> ---
>
> --- grub-mod/grub-core/commands/videoinfo.c 2022-10-12
> 13:30:54.992451000 +
> +++ grub/grub-core/commands/videoinfo.c 2022-10-12 13:31:37.715512000
> +
> @@ -30,7 +30,6 @@ GRUB_MOD_LICENSE ("GPLv3+");
> struct hook_ctx
> {
>unsigned height, width, depth;
> - unsigned char set_variable;
>struct grub_video_mode_info *current_mode;
> };
>
> @@ -39,9 +38,6 @@ hook (const struct grub_video_mode_info
> {
>struct hook_ctx *ctx = hook_arg;
>
> - if (ctx->set_variable) /* if variable should be set don't print all modes
> */
> -return 0;
> -
>if (ctx->height && ctx->width && (info->width != ctx->width ||
> info->height != ctx->height))
> return 0;
>
> @@ -136,40 +132,31 @@ grub_cmd_videoinfo (grub_command_t cmd _
>grub_video_adapter_t adapter;
>grub_video_driver_id_t id;
>struct hook_ctx ctx;
> -
> - ctx.height = ctx.width = ctx.depth = ctx.set_variable = 0;
> -
> +
> + ctx.height = ctx.width = ctx.depth = 0;
>if (argc)
> {
> - if (argc == 2 && grub_memcmp(args[0], "--set", sizeof ("--set") - 1)
> == 0 )
> - ctx.set_variable = 1;
> - else
> -{
>const char *ptr;
>ptr = args[0];
>ctx.width = grub_strtoul (ptr, &ptr, 0);
>if (grub_errno)
> - return grub_errno;
> + return grub_errno;
>if (*ptr != 'x')
> - return grub_error (GRUB_ERR_BAD_ARGUMENT,
> -N_("invalid video mode specification `%s'"),
> -args[0]);
> + return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +N_("invalid video mode specification `%s'"),
> +args[0]);
>ptr++;
>ctx.height = grub_strtoul (ptr, &ptr, 0);
>if (grub_errno)
> - return grub_errno;
> + return grub_errno;
>if (*ptr == 'x')
> - {
> -ptr++;
> -ctx.depth = grub_strtoul (ptr, &ptr, 0);
> -if (grub_errno)
> - return grub_errno;
> - }
> -}
> -
> + {
> + ptr++;
> + ctx.depth = grub_strtoul (ptr, &ptr, 0);
> + if (grub_errno)
> + return grub_errno;
> + }
> }
> -
> -
>
> #ifdef GRUB_MACHINE_PCBIOS
>if (grub_strcmp (cmd->name, "vbeinfo") == 0)
> @@ -177,23 +164,20 @@ grub_cmd_videoinfo (grub_command_t cmd _
> #endif
>
>id = grub_video_get_driver_id ();
> - if (!ctx.set_variable)
> -{
> -grub_puts_ (N_("List of supported video modes:"));
> -grub_puts_ (N_("Legend: mask/position=red/green/blue/reserved"));
> -}
> -
> +
> + grub_puts_ (N_("List of supported video modes:"));
> + grub_puts_ (N_("Legend: mask/position=red/green/blue/reserved"));
> +
>FOR_VIDEO_ADAPTERS (adapter)
>{
> struct grub_video_mode_info info;
> struct grub_video_edid_info edid_info;
> -if (!ctx.set_variable)
> - grub_printf_ (N_("Adapter `%s':\n"), adapter->name);
> +
> +grub_printf_ (N_("Adapter `%s':\n"), adapter->name);
>
> if (!adapter->iterate)
>{
> -if (!ctx.set_variable)
> -grub_puts_ (N_(" No info available"));
> + grub_puts_ (N_(" No info available"));
> continue;
>}
>
> @@ -202,18 +186,7 @@ grub_puts_ (N_("Legend: mask/position=re
> if (adapter->id == id)
>{
> if (grub_vi
Re: [PATCH v5 1/1] Add support for grub-emu to kexec Linux menu entries
Daniel Kiper writes: > On Wed, Oct 19, 2022 at 02:50:00PM -0400, Robbie Harwood wrote: > >> diff --git a/grub-core/kern/emu/misc.c b/grub-core/kern/emu/misc.c >> index d0e7a107e7..521220b49d 100644 >> --- a/grub-core/kern/emu/misc.c >> +++ b/grub-core/kern/emu/misc.c >> @@ -39,6 +39,7 @@ >> #include >> >> int verbosity; >> +int kexecute; > > We have bool type now so I would use it here. Or at least try it does > not break other builds... :-) As written, it's not actually a boolean... it keeps track of how many times the flag has been passed, and that's how the "pass twice" behavior is implemented. Be well, --Robbie signature.asc Description: PGP signature ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 2/2] video/readers/jpeg: Check next_marker is within file size
In grub-core/video/readers/jpeg.c, the function grub_jpeg_decode_huff_table()
has the variable next_marker which reads data from grub_jpeg_get_word() and
then uses it as an upper limit in a while loop. However, the function isn't
checking that next_marker is within the file size, so this check is being added
to the function.
Signed-off-by: Alec Brown
---
grub-core/video/readers/jpeg.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/grub-core/video/readers/jpeg.c b/grub-core/video/readers/jpeg.c
index 0eeea0e63..c0f95fbf9 100644
--- a/grub-core/video/readers/jpeg.c
+++ b/grub-core/video/readers/jpeg.c
@@ -199,6 +199,12 @@ grub_jpeg_decode_huff_table (struct grub_jpeg_data *data)
next_marker = data->file->offset;
next_marker += grub_jpeg_get_word (data);
+ if (next_marker > data->file->size)
+{
+ /* Should never be set beyond the size of the file. */
+ return grub_error (GRUB_ERR_BAD_FILE_TYPE, "jpeg: invalid next
reference");
+}
+
while (data->file->offset + sizeof (count) + 1 <= next_marker)
{
id = grub_jpeg_get_byte (data);
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 0/2] Fix Coverity untrusted loop bound bugs in jpeg.c
In grub-core/video/readers/jpeg.c, Coverity identified an untrusted loop bound bug. After resolving this bug, a private Coverity scan identified another untrusted loop bound bug in a different function. Since this bug only shows up after resolving the first bug, there isn't a CID for the second bug. The Coverity bugs being addressed are: CID 292450 Alec Brown (2): video/readers: Add artificial limit to image dimensions video/readers/jpeg: Check next_marker is within file size docs/grub.texi | 3 ++- grub-core/video/readers/jpeg.c | 12 +++- grub-core/video/readers/png.c | 6 +- grub-core/video/readers/tga.c | 7 +++ include/grub/bitmap.h | 2 ++ 5 files changed, 27 insertions(+), 3 deletions(-) ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 1/2] video/readers: Add artificial limit to image dimensions
In grub-core/video/readers/jpeg.c, the height and width of a JPEG image don't
have an upper limit for how big the JPEG image can be. In coverity, this is
getting flagged as an untrusted loop bound. This issue can also seen in PNG and
TGA format images as well but coverity isn't flagging it. To prevent this, the
constant IMAGE_HW_MAX_PX is being added to bitmap.h, which has a value of 16384,
to act as an artifical limit and restrict the height and width of images. This
value was picked as it is double the current max resolution size, which is 8K.
Fixes: CID 292450
Signed-off-by: Alec Brown
---
docs/grub.texi | 3 ++-
grub-core/video/readers/jpeg.c | 6 +-
grub-core/video/readers/png.c | 6 +-
grub-core/video/readers/tga.c | 7 +++
include/grub/bitmap.h | 2 ++
5 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/docs/grub.texi b/docs/grub.texi
index 9835c878a..d448ca969 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -1507,7 +1507,8 @@ resolution. @xref{gfxmode}.
Set a background image for use with the @samp{gfxterm} graphical terminal.
The value of this option must be a file readable by GRUB at boot time, and
it must end with @file{.png}, @file{.tga}, @file{.jpg}, or @file{.jpeg}.
-The image will be scaled if necessary to fit the screen.
+The image will be scaled if necessary to fit the screen. Image height and
+width will be restricted by an artificial limit of 16384.
@item GRUB_THEME
Set a theme for use with the @samp{gfxterm} graphical terminal.
diff --git a/grub-core/video/readers/jpeg.c b/grub-core/video/readers/jpeg.c
index e31602f76..0eeea0e63 100644
--- a/grub-core/video/readers/jpeg.c
+++ b/grub-core/video/readers/jpeg.c
@@ -301,7 +301,11 @@ grub_jpeg_decode_sof (struct grub_jpeg_data *data)
data->image_height = grub_jpeg_get_word (data);
data->image_width = grub_jpeg_get_word (data);
- if ((!data->image_height) || (!data->image_width))
+ grub_dprintf ("jpeg", "image height: %d\n", data->image_height);
+ grub_dprintf ("jpeg", "image width: %d\n", data->image_width);
+
+ if ((!data->image_height) || (!data->image_width) ||
+ (data->image_height > IMAGE_HW_MAX_PX) || (data->image_width >
IMAGE_HW_MAX_PX))
return grub_error (GRUB_ERR_BAD_FILE_TYPE, "jpeg: invalid image size");
cc = grub_jpeg_get_byte (data);
diff --git a/grub-core/video/readers/png.c b/grub-core/video/readers/png.c
index 54dfedf43..d658cbb0e 100644
--- a/grub-core/video/readers/png.c
+++ b/grub-core/video/readers/png.c
@@ -252,7 +252,11 @@ grub_png_decode_image_header (struct grub_png_data *data)
data->image_width = grub_png_get_dword (data);
data->image_height = grub_png_get_dword (data);
- if ((!data->image_height) || (!data->image_width))
+ grub_dprintf ("png", "image height: %d\n", data->image_height);
+ grub_dprintf ("png", "image width: %d\n", data->image_width);
+
+ if ((!data->image_height) || (!data->image_width) ||
+ (data->image_height > IMAGE_HW_MAX_PX) || (data->image_width >
IMAGE_HW_MAX_PX))
return grub_error (GRUB_ERR_BAD_FILE_TYPE, "png: invalid image size");
color_bits = grub_png_get_byte (data);
diff --git a/grub-core/video/readers/tga.c b/grub-core/video/readers/tga.c
index a9ec3a1b6..f2f563d06 100644
--- a/grub-core/video/readers/tga.c
+++ b/grub-core/video/readers/tga.c
@@ -340,6 +340,13 @@ grub_video_reader_tga (struct grub_video_bitmap **bitmap,
data.image_width = grub_le_to_cpu16 (data.hdr.image_width);
data.image_height = grub_le_to_cpu16 (data.hdr.image_height);
+ grub_dprintf ("tga", "image height: %d\n", data.image_height);
+ grub_dprintf ("tga", "image width: %d\n", data.image_width);
+
+ /* Check image height and width are within restrictions */
+ if ((data.image_height > IMAGE_HW_MAX_PX) || (data.image_width >
IMAGE_HW_MAX_PX))
+return grub_error (GRUB_ERR_BAD_FILE_TYPE, "tga: invalid image size");
+
/* Check that bitmap encoding is supported. */
switch (data.hdr.image_type)
{
diff --git a/include/grub/bitmap.h b/include/grub/bitmap.h
index 5728f8ca3..149d37bfe 100644
--- a/include/grub/bitmap.h
+++ b/include/grub/bitmap.h
@@ -24,6 +24,8 @@
#include
#include
+#define IMAGE_HW_MAX_PX16384
+
struct grub_video_bitmap
{
/* Bitmap format description. */
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v7 1/1] plainmount: Support plain encryption mode
On Sun, 18 Sep 2022 19:46:03 +
Maxim Fomin wrote:
> From 687f8687f7c72ac91e250c9b89659f69b3644bfb Mon Sep 17 00:00:00 2001
> From: Maxim Fomin
> Date: Sun, 18 Sep 2022 19:43:12 +0100
> Subject: [PATCH v7 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 | 457
> 3 files changed, 542 insertions(+)
> create mode 100644 grub-core/disk/plainmount.c
Sorry I couldn't get to this sooner, its been a busy month.
Since this is a fairly large patch, it would make my life easier if you
would use the --interdiff or --range-diff, which ever is easier for
you. This way I can just re-review the changes to each patch update.
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 107f66ebc..82e79a900 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -4267,6 +4267,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
> @@ -4554,6 +4555,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
> @@ -5113,6 +5122,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
s/with/using/
> +syntax and loopback device. The following example shows how to specify 1MiB
> +offset:
> +
> +@example
> +loopback node (hd0,gpt1)2048+
> +plainmount node
I'm noticing that this could be confusing because technically
plainmount must specify -c and -s options. Perhaps something like
"plainmount node @var{...}" would be better.
> +@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 a password: a keyfile and a secret passphrase.
> +The keyfile path parameter has higher priority than the secret passphrase
> +parameter and is specified with the option @option{-d}. Password data
> obtained
> +from keyfiles is not hashed and is used directly as a cipher key. An optional
> +offset of password data in the keyfile can be specified with the option
> +@option{-O} or directly with the option @option{-d} and GRUB blocklist
> syntax.
s/./, if the keyfile data can be accessed from a device and is 512
byte aligned./
> +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
Ditto above about adding @var{...}.
> +@end example
> +
> +If no keyfile is specified then the password is set to the string specified
> +by 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 @samp{plain} which means that no hashing is done and such
> +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, @samp{aes-xts-plain64}). Key
> size
> +option @option{-s} is the key size of the cipher in bits, not to be confused
> with
> +the offset of the key data in a keyfile specified wit
[PATCH v2] cmp: Only return success when both files have the same contents
From: Li Gen
This allows the cmp command to be used in GRUB scripts to conditionally
run commands based on whether two files are the same.
Update documentation accordingly.
Signed-off-by: Li Gen
Signed-off-by: Glenn Washburn
---
Range-diff against v1:
1: 5832522ca2 ! 1: 3e6c866d87 cmp: Only return success when both files have
the same contents
@@ Commit message
This allows the cmp command to be used in GRUB scripts to conditionally
run commands based on whether two files are the same.
+Update documentation accordingly.
+
Signed-off-by: Li Gen
Signed-off-by: Glenn Washburn
+ ## docs/grub.texi ##
+@@ docs/grub.texi: bytes like this:
+ Differ at the offset 777: 0xbe [foo], 0xef [bar]
+ @end example
+
+-If they are completely identical, nothing will be printed.
++If they are completely identical, nothing will be printed, and @code{$?}
++will be set to 0. Otherwise, if the files are not identical, @code{$?}
++will be set to a nonzero value.
+ @end deffn
+
+
+
## grub-core/commands/cmp.c ##
@@ grub-core/commands/cmp.c: grub_cmd_cmp (grub_command_t cmd
__attribute__ ((unused)),
grub_file_t file2 = 0;
docs/grub.texi | 4 +++-
grub-core/commands/cmp.c | 4 +++-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/docs/grub.texi b/docs/grub.texi
index 4a6fb1a727..b12f47d351 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -4486,7 +4486,9 @@ bytes like this:
Differ at the offset 777: 0xbe [foo], 0xef [bar]
@end example
-If they are completely identical, nothing will be printed.
+If they are completely identical, nothing will be printed, and @code{$?}
+will be set to 0. Otherwise, if the files are not identical, @code{$?}
+will be set to a nonzero value.
@end deffn
diff --git a/grub-core/commands/cmp.c b/grub-core/commands/cmp.c
index e9c3b25d34..e1665cf27b 100644
--- a/grub-core/commands/cmp.c
+++ b/grub-core/commands/cmp.c
@@ -38,6 +38,7 @@ grub_cmd_cmp (grub_command_t cmd __attribute__ ((unused)),
grub_file_t file2 = 0;
char *buf1 = 0;
char *buf2 = 0;
+ grub_err_t err = GRUB_ERR_TEST_FAILURE;
if (argc != 2)
return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("two arguments expected"));
@@ -91,6 +92,7 @@ grub_cmd_cmp (grub_command_t cmd __attribute__ ((unused)),
/* TRANSLATORS: it's always exactly 2 files. */
grub_printf_ (N_("The files are identical.\n"));
+ err = GRUB_ERR_NONE;
}
cleanup:
@@ -102,7 +104,7 @@ cleanup:
if (file2)
grub_file_close (file2);
- return grub_errno;
+ return err;
}
static grub_command_t cmd;
--
2.34.1
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
