Re: [PATCH v2 0/8] serial: Add MMIO & SPCR support
On Thu, 2023-01-19 at 18:31 +0100, Daniel Kiper wrote:
>
> Sadly your patch set broke various builds including x86-ieee1275 one. I had
> to do some tweaks to make it work again. You can find most important ones
> below. Please double check I have not messed up something.
Oh, sorry about that. I didn't know there even was such a thing as x86-
ieee1275. Is there an easy way to run / test all those builds ? A
script or service somewhere ?
> Daniel
>
> diff --git a/grub-core/term/ieee1275/serial.c
> b/grub-core/term/ieee1275/serial.c
> index afbe8dcda..0e4cac4c4 100644
> --- a/grub-core/term/ieee1275/serial.c
> +++ b/grub-core/term/ieee1275/serial.c
> @@ -228,7 +228,7 @@ add_port (struct ofserial_hash_ent *ent)
> return NULL;
>
> FOR_SERIAL_PORTS (port)
> - if (port->ent == ent)
> + if (port->elem == ent)
> return port;
Right.
> port = grub_zalloc (sizeof (*port));
> @@ -252,7 +252,7 @@ add_port (struct ofserial_hash_ent *ent)
> return port;
> }
>
> -const struct grub_serial_port *
> +struct grub_serial_port *
> grub_ofserial_add_port (const char *path)
> {
> struct ofserial_hash_ent *ent;
> diff --git a/grub-core/term/ns8250-spcr.c b/grub-core/term/ns8250-spcr.c
> index 01533d969..d101bffb5 100644
> --- a/grub-core/term/ns8250-spcr.c
> +++ b/grub-core/term/ns8250-spcr.c
> @@ -16,6 +16,8 @@
> * along with GRUB. If not, see
Re: [PATCH v2 0/5] fs/iso9660: Fix out-of-bounds read
On Jan 19, 2023, at 3:58 AM, Thomas Schmitt
mailto:scdbac...@gmx.net>> wrote:
Hi,
i wrote:
libisofs and xorriso are in the process of getting an adjustable curb to
prevent the production of ISO filesystems with files which would not show
up in Linux. I decided for 100,000 hops as hard limit but set the default
to 31.
Lidong Chen wrote:
I am not sure I understand the hard limit vs the default in terms of
checking the number of hops.
xorriso produces and reads ISO 9660 filesystems.
At production time, the limit will be adjustable. The default will be 31
to prevent files which don't show up when the filesystem is mounted by
Linux. The maximum adjustable limit at production time will be 100,000.
At read time, e.g. for extracting files or for adding another session to
the ISO, the limit will be 100,000.
Since the limit of CE hops has been decided,
I meanwhile deem the proposed 1 million allowed hops quite high.
An opinion by experienced GRUB developers would be helpful.
if the previous proposed fix still stands, I can create a patch for it.
The fix by hop counting is the right thing to do.
The tough part for me is the testing.
You can use for testing
https://urldefense.com/v3/__http://scdbackup.webframe.org/ce_loop.iso.gz__;!!ACWV5N9M2RV99hQ!OXku0fFl7fyFeTon9H7cdACNDHk8OrsvozKrR5iTCzbjwS90Ys6gZI8kpfDlpAgRKh2q3YKxK7ROWT5K7cg$
SHA256: d86b73b0cc260968f50c30a5207b798f5fc2396233aee5eb3cedf9cef069f3c2
and
https://urldefense.com/v3/__http://scdbackup.webframe.org/ce_loop2.iso.gz__;!!ACWV5N9M2RV99hQ!OXku0fFl7fyFeTon9H7cdACNDHk8OrsvozKrR5iTCzbjwS90Ys6gZI8kpfDlpAgRKh2q3YKxK7RO7e3E4Lw$
SHA256: a6bde0c1562de8959d783bca0a79ad750da2bc129bdea2362b4a7c3e83426b2c
ce_loop.iso currently causes no endless loop in grub-fstest, because
the CE entry at the start of the (bad) continuation area is ignored,
against the prescriptions of SUSP.
It will cause an endless loop after patch 5/5 is applied and the
self-pointing CE entry is not ignored any more by mistake.
./grub-fstest ce_loop.iso ls /
ce_loop2.iso already now causes an endless loop with
./grub-fstest ce_loop2.iso ls /
Both endless loops should be detected and cause a GRUB error when the
CE hop counter and loop breaker is in effect.
I ran grub-fastest with both ce_loop ISO files. The endless loops were detected
and Grub exited accordingly. I didn't know where the grub error message
were stored in case of grub-fastest. But, I traced with gdb, and saw the
code reported the error. If the diff looks good, I will send the v3 patches set.
+#define GRUB_ISO9660_MAX_CE_HOPS 10
struct grub_iso9660_susp_entry *entry;
grub_err_t err;
+ int ce_counter = 0;
struct grub_iso9660_susp_ce *ce;
grub_disk_addr_t ce_block;
+ if (++ce_counter > GRUB_ISO9660_MAX_CE_HOPS)
+ {
+ grub_free (sua);
+ return grub_error (GRUB_ERR_BAD_FS,
+"suspecting endless CE loop");
+ }
+
ce = (struct grub_iso9660_susp_ce *) entry;
entry = (struct grub_iso9660_susp_entry *) sua;
+ /*
+ * The hook function will not process CE or ST.
+ * Advancing to the next entry would skip them.
+ */
+ if (grub_strncmp ((char *) entry->sig, "CE", 2) == 0
+ || grub_strncmp ((char *) entry->sig, "ST", 2) == 0)
+ continue;
}
if (hook (entry, hook_arg))
Thanks,
Lidong
(I can meanwhile provide ISOs which have 32+ CE hops without loop, i.e.
righteously storing 64+ KiB of data in the chain of SUSP entries of a
file. But that's mainly interesting for testing Linux, not for GRUB.)
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2 0/8] serial: Add MMIO & SPCR support
On Thu, Jan 12, 2023 at 01:51:56PM +0100, Daniel Kiper wrote:
> On Fri, Dec 23, 2022 at 12:47:51PM +1100, Benjamin Herrenschmidt wrote:
> > This series adds support for MMIO serial ports and auto-configuration
> > via ACPI SPCR.
> >
> > This is necessary for the serial port to work on AWS EC2 "metal" x86
> > systems.
> >
> > An MMIO serial port can be specified explicitely using the
> > "mmio," prefix for the --port argument to the 'serial' command,
> > the register access size can also be specified via a suffix,
> > and when 'serial' is used without arguments, it will try to use
> > an ACPI SPCR entry (if any) to add the default port and configure
> > it appropriately (speed, parity etc...)
> >
> > This was tested using SPCR on an actual c5.metal instance, and using
> > explicit instanciation via serial -p mmio on a modified qemu
> > hacked to create MMIO PCI serial ports and to create a SPCR ACPI table.
> >
> > The insipiration was an original implementation by
> > Matthias Lange ! matthias.lange at kernkonzept.com
> > However, the code was rewritten pretty much from scratch.
>
> For all the patches Reviewed-by: Daniel Kiper ...
Sadly your patch set broke various builds including x86-ieee1275 one. I had
to do some tweaks to make it work again. You can find most important ones
below. Please double check I have not messed up something.
Daniel
diff --git a/grub-core/term/ieee1275/serial.c b/grub-core/term/ieee1275/serial.c
index afbe8dcda..0e4cac4c4 100644
--- a/grub-core/term/ieee1275/serial.c
+++ b/grub-core/term/ieee1275/serial.c
@@ -228,7 +228,7 @@ add_port (struct ofserial_hash_ent *ent)
return NULL;
FOR_SERIAL_PORTS (port)
-if (port->ent == ent)
+if (port->elem == ent)
return port;
port = grub_zalloc (sizeof (*port));
@@ -252,7 +252,7 @@ add_port (struct ofserial_hash_ent *ent)
return port;
}
-const struct grub_serial_port *
+struct grub_serial_port *
grub_ofserial_add_port (const char *path)
{
struct ofserial_hash_ent *ent;
diff --git a/grub-core/term/ns8250-spcr.c b/grub-core/term/ns8250-spcr.c
index 01533d969..d101bffb5 100644
--- a/grub-core/term/ns8250-spcr.c
+++ b/grub-core/term/ns8250-spcr.c
@@ -16,6 +16,8 @@
* along with GRUB. If not, see
Re: [PATCH v4 1/2] mm: Adjust new region size to take management overhead into account
I looked at this patch again and found a few more (minor) issues...
On Sat, Jan 14, 2023 at 09:23:22PM +0800, Zhang Boyang wrote:
> When grub_memalign() encounters out-of-memory, it will try
> grub_mm_add_region_fn() to request more memory from system firmware.
> However, the size passed to it doesn't take region management overhead
> into account. Adding a memory area of "size" bytes may result in a heap
> region of less than "size" bytes truely avaliable. Thus, the new region
> may not be adequate for current allocation request, confusing
> out-of-memory handling code.
>
> This patch introduces GRUB_MM_MGMT_OVERHEAD to address the region
> management overhead (e.g. metadata, padding). The value of this new
> constant must be large enough to make sure grub_malloc(size) always
> success after a successful call to grub_mm_init_region(addr, size +
> GRUB_MM_MGMT_OVERHEAD), for any given addr and size (assuming no
> interger overflow).
>
> The size passed to grub_mm_add_region_fn() is now correctly adjusted,
> thus if grub_mm_add_region_fn() succeeded, current allocation request
> can always success.
>
> Signed-off-by: Zhang Boyang
> ---
> grub-core/kern/mm.c | 64 ++---
> 1 file changed, 61 insertions(+), 3 deletions(-)
>
> diff --git a/grub-core/kern/mm.c b/grub-core/kern/mm.c
> index ae2279133..278756dea 100644
> --- a/grub-core/kern/mm.c
> +++ b/grub-core/kern/mm.c
> @@ -83,6 +83,46 @@
>
>
>
> +/*
> + * GRUB_MM_MGMT_OVERHEAD is an upper bound of management overhead of
> + * each region, with any possible padding taken into account.
> + *
> + * The value must be large enough to make sure grub_malloc(size) always
> + * success after a successful call to
> + * grub_mm_init_region(addr, size + GRUB_MM_MGMT_OVERHEAD), for any given
> + * addr and size (assuming no interger overflow).
> + *
> + * The worst case which has maximum overhead is shown in the figure below:
> + *
> + * +-- addr
> + * v |<- size ->|
> + * +-+++--+-+
> + * | padding | grub_mm_region | grub_mm_header | usable bytes | padding |
> + * +-+++--+-+
> + * |<- a ->|<- b ->|<- c ->|<-d ->|<- e ->|
This drawing is missing grub_memalign() align argument. It should be
put between "c" and "d". Though the code below of course takes it into
account... :-)
> + *b == sizeof (struct grub_mm_region) +--/ This will be the pointer
> + *c == sizeof (struct grub_mm_header) | returned by next
> + *d == size | grub_malloc(size),
> + *| if no other suitable free
> + * Assuming addr % GRUB_MM_ALIGN == 1, then \ block is available.
> + *a == GRUB_MM_ALIGN - 1
> + *
> + * Assuming size % GRUB_MM_ALIGN == 1, then
> + *e == GRUB_MM_ALIGN - 1
> + *
> + * Therefore, the maximum overhead is:
> + *a + b + c + e == (GRUB_MM_ALIGN - 1) + sizeof (struct grub_mm_region)
> + * + sizeof (struct grub_mm_header) + (GRUB_MM_ALIGN - 1)
> + */
> +#define GRUB_MM_MGMT_OVERHEAD((GRUB_MM_ALIGN - 1) \
> + + sizeof (struct grub_mm_region) \
> + + sizeof (struct grub_mm_header) \
> + + (GRUB_MM_ALIGN - 1))
> +
> +/* The size passed to grub_mm_add_region_fn() is aligned up by this value. */
> +#define GRUB_MM_HEAP_GROW_ALIGN 4096
> +
> grub_mm_region_t grub_mm_base;
> grub_mm_add_region_func_t grub_mm_add_region_fn;
>
> @@ -230,6 +270,11 @@ grub_mm_init_region (void *addr, grub_size_t size)
>
>grub_dprintf ("regions", "No: considering a new region at %p of size %"
> PRIxGRUB_SIZE "\n",
> addr, size);
> + /*
> + * If you want to modify the code below, please also take a look at
> + * GRUB_MM_MGMT_OVERHEAD and make sure it is synchronized with the code.
> + */
> +
>/* Allocate a region from the head. */
>r = (grub_mm_region_t) ALIGN_UP ((grub_addr_t) addr, GRUB_MM_ALIGN);
>
> @@ -410,6 +455,7 @@ 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 grow;
>int count = 0;
>
>if (!grub_mm_base)
> @@ -418,10 +464,22 @@ grub_memalign (grub_size_t align, grub_size_t size)
>if (size > ~(grub_size_t) align)
> goto fail;
>
> + /*
> + * Pre-calculate the necessary size of heap growth (if applicable),
> + * with region management overhead taken into account.
> + */
> + if (grub_add (size + align, GRUB_MM_MGMT_OVERHEAD, ))
> +goto fail;
> +
> + /* Align up heap growth to make it friendly to CPU/MMU. */
> + if (grow > ~(grub_size_t) (GRUB_MM_HEAP_GROW_ALIGN - 1))
> +goto fail;
> + grow = ALIGN_UP (grow, GRUB_MM_HEAP_GROW_ALIGN);
Re: [PATCH v2 0/5] fs/iso9660: Fix out-of-bounds read
Hi, i wrote: > > libisofs and xorriso are in the process of getting an adjustable curb to > > prevent the production of ISO filesystems with files which would not show > > up in Linux. I decided for 100,000 hops as hard limit but set the default > > to 31. Lidong Chen wrote: > I am not sure I understand the hard limit vs the default in terms of > checking the number of hops. xorriso produces and reads ISO 9660 filesystems. At production time, the limit will be adjustable. The default will be 31 to prevent files which don't show up when the filesystem is mounted by Linux. The maximum adjustable limit at production time will be 100,000. At read time, e.g. for extracting files or for adding another session to the ISO, the limit will be 100,000. > Since the limit of CE hops has been decided, I meanwhile deem the proposed 1 million allowed hops quite high. An opinion by experienced GRUB developers would be helpful. > if the previous proposed fix still stands, I can create a patch for it. The fix by hop counting is the right thing to do. > The tough part for me is the testing. You can use for testing http://scdbackup.webframe.org/ce_loop.iso.gz SHA256: d86b73b0cc260968f50c30a5207b798f5fc2396233aee5eb3cedf9cef069f3c2 and http://scdbackup.webframe.org/ce_loop2.iso.gz SHA256: a6bde0c1562de8959d783bca0a79ad750da2bc129bdea2362b4a7c3e83426b2c ce_loop.iso currently causes no endless loop in grub-fstest, because the CE entry at the start of the (bad) continuation area is ignored, against the prescriptions of SUSP. It will cause an endless loop after patch 5/5 is applied and the self-pointing CE entry is not ignored any more by mistake. ./grub-fstest ce_loop.iso ls / ce_loop2.iso already now causes an endless loop with ./grub-fstest ce_loop2.iso ls / Both endless loops should be detected and cause a GRUB error when the CE hop counter and loop breaker is in effect. (I can meanwhile provide ISOs which have 32+ CE hops without loop, i.e. righteously storing 64+ KiB of data in the chain of SUSP entries of a file. But that's mainly interesting for testing Linux, not for GRUB.) Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
