SeaBIOS January 2022

seabios@seabios.org

16 participants
15 discussions

[PATCH] nvme: avoid use-after-free in nvme_controller_enable()

by Jan Beulich

Commit b68f313c9139 ("nvme: Record maximum allowed request size") introduced a use of "identify" past it being passed to free(). Latch the value of interest into a local variable. Reported-by: Coverity (ID 1497613) Signed-off-by: Jan Beulich <jbeulich(a)suse.com> --- It was a Xen Project Coverity run which reported this after our updating to 1.15.0. --- a/src/hw/nvme.c +++ b/src/hw/nvme.c @@ -620,6 +620,7 @@ identify->nn, (identify->nn == 1) ? "" : "s"); ctrl->ns_count = identify->nn; + u8 mdts = identify->mdts; free(identify); if ((ctrl->ns_count == 0) || nvme_create_io_queues(ctrl)) { @@ -631,7 +632,7 @@ /* Populate namespace IDs */ int ns_idx; for (ns_idx = 0; ns_idx < ctrl->ns_count; ns_idx++) { - nvme_probe_ns(ctrl, ns_idx, identify->mdts); + nvme_probe_ns(ctrl, ns_idx, mdts); } dprintf(3, "NVMe initialization complete!\n");

3 days, 5 hours

[PATCH] nvme: fix LBA format data structure

by Florian Larysch

The LBA Format Data structure is dword-sized, but struct nvme_lba_format erroneously contains an additional member, misaligning all LBAF descriptors after the first and causing them to be misinterpreted. Remove it. Signed-off-by: Florian Larysch <fl(a)n621.de> --- src/hw/nvme-int.h | 1 - 1 file changed, 1 deletion(-) diff --git a/src/hw/nvme-int.h b/src/hw/nvme-int.h index a4c1555..5779203 100644 --- a/src/hw/nvme-int.h +++ b/src/hw/nvme-int.h @@ -156,7 +156,6 @@ struct nvme_lba_format { u16 ms; u8 lbads; u8 rp; - u8 res; }; struct nvme_identify_ns { -- 2.34.1

3 days, 21 hours

[PATCH] sercon: Fix missing GET_LOW() to access rx_bytes

by Kevin O'Connor

The variable rx_bytes is marked VARLOW, but there was a missing GET_LOW() to access rx_bytes. Fix by copying rx_bytes to a local variable and avoid the repetitive segment memory accesses. Reported-by: Gabe Black <gabe.black(a)gmail.com> Signed-off-by: Volker Rümelin <vr_qemu(a)t-online.de> Signed-off-by: Kevin O'Connor <kevin(a)koconnor.net> --- src/sercon.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/sercon.c b/src/sercon.c index 66a1f24..3019d9b 100644 --- a/src/sercon.c +++ b/src/sercon.c @@ -626,7 +626,7 @@ sercon_check_event(void) u16 addr = GET_LOW(sercon_port); u16 keycode; u8 byte, count = 0; - int seq, chr; + int seq; // check to see if there is a active serial port if (!addr) @@ -640,20 +640,23 @@ sercon_check_event(void) // read all available data while (inb(addr + SEROFF_LSR) & 0x01) { byte = inb(addr + SEROFF_DATA); - if (GET_LOW(rx_bytes) < sizeof(rx_buf)) { - SET_LOW(rx_buf[rx_bytes], byte); - SET_LOW(rx_bytes, GET_LOW(rx_bytes) + 1); + u8 rb = GET_LOW(rx_bytes); + if (rb < sizeof(rx_buf)) { + SET_LOW(rx_buf[rb], byte); + SET_LOW(rx_bytes, rb + 1); count++; } } for (;;) { // no (more) input data - if (!GET_LOW(rx_bytes)) + u8 rb = GET_LOW(rx_bytes); + if (!rb) return; // lookup escape sequences - if (GET_LOW(rx_bytes) > 1 && GET_LOW(rx_buf[0]) == 0x1b) { + u8 next_char = GET_LOW(rx_buf[0]); + if (rb > 1 && next_char == 0x1b) { seq = findseq(); if (seq >= 0) { enqueue_key(GET_GLOBAL(termseq[seq].keycode)); @@ -664,12 +667,11 @@ sercon_check_event(void) // Seems we got a escape sequence we didn't recognise. // -> If we received data wait for more, maybe it is just incomplete. - if (GET_LOW(rx_buf[0]) == 0x1b && count) + if (next_char == 0x1b && count) return; // Handle input as individual char. - chr = GET_LOW(rx_buf[0]); - keycode = ascii_to_keycode(chr); + keycode = ascii_to_keycode(next_char); if (keycode) enqueue_key(keycode); shiftbuf(1); -- 2.31.1

4 days, 19 hours

[PATCH 0/5] Rework NVME page transfers and avoid fsegment allocation

by Kevin O'Connor

Hi Alex, I was looking at your recent fix for SeaBIOS NVME. I agree that it is not valid to write to the f-segment during runtime. However, I was struggling to understand the PRP list management in the nvme.c code. I came up with a different implementation that avoids allocating another buffer in the "high zone". Instead, the code in this patch series builds the page list in the existing "dma bounce buffer". I don't have a good way to test this on real hardware. It passes simple qemu tests (both with and without kvm). For example: qemu-system-x86_64 -k en-us -snapshot -L test -chardev stdio,id=seabios -device isa-debugcon,iobase=0x402,chardev=seabios -m 512 -drive file=dos-drivec,if=none,id=drive0 -device nvme,drive=drive0,serial=nvme-1 -boot menu=on Thanks, -Kevin Kevin O'Connor (5): nvme: Rework nvme_io_readwrite() to return -1 on error nvme: Add nvme_bounce_xfer() helper function nvme: Convert nvme_build_prpl() to nvme_prpl_xfer() nvme: Pass prp1 and prp2 directly to nvme_io_xfer() nvme: Build the page list in the existing dma buffer src/hw/nvme-int.h | 7 --- src/hw/nvme.c | 143 ++++++++++++++++++++-------------------------- 2 files changed, 61 insertions(+), 89 deletions(-) -- 2.31.1

4 days, 20 hours

[PATCHv2 0/6] Rework NVME page transfers and avoid fsegment allocation

by Kevin O'Connor

This is a resend of the previous series. Changes since last time: * Patch 1: Fix function comment on nvme_io_xfer() * Patch 3-5: Added "goto bounce" to nvme_io_xfer() as suggested by Alex * Patch 6: Added separate "single dma bounce buffer" patch to this series * Patch 6: Add checking for malloc failure -Kevin Kevin O'Connor (6): nvme: Rework nvme_io_readwrite() to return -1 on error nvme: Add nvme_bounce_xfer() helper function nvme: Convert nvme_build_prpl() to nvme_prpl_xfer() nvme: Pass prp1 and prp2 directly to nvme_io_xfer() nvme: Build the page list in the existing dma buffer nvme: Only allocate one dma bounce buffer for all nvme drives src/hw/nvme-int.h | 10 --- src/hw/nvme.c | 163 ++++++++++++++++++++++------------------------ 2 files changed, 78 insertions(+), 95 deletions(-) -- 2.31.1

5 days, 17 hours

[PATCH] smm: Suppress gcc array-bounds warnings

by Kevin O'Connor

Add a hack to suppress spurious gcc array-bounds warning. Signed-off-by: Kevin O'Connor <kevin(a)koconnor.net> --- src/fw/smm.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/src/fw/smm.c b/src/fw/smm.c index d90e43a..a0b50b2 100644 --- a/src/fw/smm.c +++ b/src/fw/smm.c @@ -59,6 +59,14 @@ struct smm_layout { struct smm_state cpu; }; +// Hack to supress some gcc array-bounds warnings +static void +memcpy_nowarn(void *d, const void *s, size_t len) +{ + asm("" : "+r"(d), "+r"(s)); + memcpy(d, s, len); +} + void VISIBLE32FLAT handle_smi(u16 cs) { @@ -85,8 +93,8 @@ handle_smi(u16 cs) if (CONFIG_CALL32_SMM) { // Backup current cpu state for SMM trampolining struct smm_layout *newsmm = (void*)BUILD_SMM_ADDR; - memcpy(&newsmm->backup1, &smm->cpu, sizeof(newsmm->backup1)); - memcpy(&newsmm->backup2, &smm->cpu, sizeof(newsmm->backup2)); + memcpy_nowarn(&newsmm->backup1, &smm->cpu, sizeof(newsmm->backup1)); + memcpy_nowarn(&newsmm->backup2, &smm->cpu, sizeof(newsmm->backup2)); HaveSmmCall32 = 1; } @@ -145,8 +153,8 @@ smm_save_and_copy(void) // save original memory content struct smm_layout *initsmm = (void*)BUILD_SMM_INIT_ADDR; struct smm_layout *smm = (void*)BUILD_SMM_ADDR; - memcpy(&smm->cpu, &initsmm->cpu, sizeof(smm->cpu)); - memcpy(&smm->codeentry, &initsmm->codeentry, sizeof(smm->codeentry)); + memcpy_nowarn(&smm->cpu, &initsmm->cpu, sizeof(smm->cpu)); + memcpy_nowarn(&smm->codeentry, &initsmm->codeentry, sizeof(smm->codeentry)); // Setup code entry point. initsmm->codeentry = SMI_INSN; @@ -168,8 +176,9 @@ smm_relocate_and_restore(void) /* restore original memory content */ struct smm_layout *initsmm = (void*)BUILD_SMM_INIT_ADDR; struct smm_layout *smm = (void*)BUILD_SMM_ADDR; - memcpy(&initsmm->cpu, &smm->cpu, sizeof(initsmm->cpu)); - memcpy(&initsmm->codeentry, &smm->codeentry, sizeof(initsmm->codeentry)); + memcpy_nowarn(&initsmm->cpu, &smm->cpu, sizeof(initsmm->cpu)); + memcpy_nowarn(&initsmm->codeentry, &smm->codeentry + , sizeof(initsmm->codeentry)); // Setup code entry point. smm->codeentry = SMI_INSN; -- 2.31.1

5 days, 22 hours

[PATCH] nvme: Only allocate one dma bounce buffer for all nvme drives

by Kevin O'Connor

There is no need to create multiple dma bounce buffers as the BIOS disk code isn't reentrant capable. Signed-off-by: Kevin O'Connor <kevin(a)koconnor.net> --- This is a minor cleanup on top of the previous nvme code series. -Kevin --- src/hw/nvme-int.h | 3 --- src/hw/nvme.c | 14 +++++++++----- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/hw/nvme-int.h b/src/hw/nvme-int.h index f0d717d..f9c807e 100644 --- a/src/hw/nvme-int.h +++ b/src/hw/nvme-int.h @@ -117,9 +117,6 @@ struct nvme_namespace { u32 block_size; u32 metadata_size; u32 max_req_size; - - /* Page aligned buffer of size NVME_PAGE_SIZE. */ - char *dma_buffer; }; /* Data structures for NVMe admin identify commands */ diff --git a/src/hw/nvme.c b/src/hw/nvme.c index 39b9138..0ba981c 100644 --- a/src/hw/nvme.c +++ b/src/hw/nvme.c @@ -20,6 +20,9 @@ #include "nvme.h" #include "nvme-int.h" +// Page aligned "dma bounce buffer" of size NVME_PAGE_SIZE in high memory +static void *nvme_dma_buffer; + static void * zalloc_page_aligned(struct zone_s *zone, u32 size) { @@ -294,7 +297,8 @@ nvme_probe_ns(struct nvme_ctrl *ctrl, u32 ns_idx, u8 mdts) ns->max_req_size = -1U; } - ns->dma_buffer = zalloc_page_aligned(&ZoneHigh, NVME_PAGE_SIZE); + if (!nvme_dma_buffer) + nvme_dma_buffer = zalloc_page_aligned(&ZoneHigh, NVME_PAGE_SIZE); char *desc = znprintf(MAXDESCSIZE, "NVMe NS %u: %llu MiB (%llu %u-byte " "blocks + %u-byte metadata)", @@ -460,12 +464,12 @@ nvme_bounce_xfer(struct nvme_namespace *ns, u64 lba, void *buf, u16 count, u16 blocks = count < max_blocks ? count : max_blocks; if (write) - memcpy(ns->dma_buffer, buf, blocks * ns->block_size); + memcpy(nvme_dma_buffer, buf, blocks * ns->block_size); - int res = nvme_io_xfer(ns, lba, ns->dma_buffer, NULL, blocks, write); + int res = nvme_io_xfer(ns, lba, nvme_dma_buffer, NULL, blocks, write); if (!write && res >= 0) - memcpy(buf, ns->dma_buffer, res * ns->block_size); + memcpy(buf, nvme_dma_buffer, res * ns->block_size); return res; } @@ -499,7 +503,7 @@ nvme_prpl_xfer(struct nvme_namespace *ns, u64 lba, void *buf, u16 count, if ((ns->block_size * count) > (NVME_PAGE_SIZE * 2)) { /* We need to describe more than 2 pages, build PRP List */ u32 prpl_len = 0; - u64 *prpl = (void*)ns->dma_buffer; + u64 *prpl = nvme_dma_buffer; int first_page = 1; for (; size > 0; base += NVME_PAGE_SIZE, size -= NVME_PAGE_SIZE) { if (first_page) { -- 2.31.1

5 days, 23 hours

Re: MP tables do not report multiple CPUs in Qemu 6.2.0 on x86 when given -smp cpus=n flag

by Igor Mammedov

On Wed, 19 Jan 2022 15:48:20 +0000 Peter Maydell <peter.maydell(a)linaro.org> wrote: > On Wed, 19 Jan 2022 at 14:44, Godmar Back <gback(a)cs.vt.edu> wrote: > > after upgrading to 6.2.0, I observed that code such as MIT's xv6 (see > > [1]) is no longer able to detect multiple CPUs. Their code works in > > 6.1.1, however. > > Hi; this isn't a great place for reporting QEMU bugs, because > it's more of a user-to-user discussion list. Not all QEMU > developers read it. I've cc'd the ACPI maintainers, who > hopefully may have an idea about what's happening here. > You could also file a bug at > https://gitlab.com/qemu-project/qemu/-/issues > > > I built 6.1.1 from source and 6.2.0 from source and I have also tested > > with CentOS stream's 6.1.1 qemu-kvm and was able to pinpoint this > > change to these 2 versions of qemu. I am using qemu-system-i386 > > specifically. > > > > I tried to go through the ChangeLog to see if the `-smp` option was > > deprecated or changed. I found this note [2] about invalid topologies > > having been removed in 5.2. Here's what I found after long > > experimentation: > > > > The legacy MP tables appear to work only if you specify the longform > > `-smp cpus=4,cores=1,threads=1,sockets=4` in 6.2.0. If you specify > > `-smp 4` or `-smp cpus=4` it will not work in 6.2.0 (it worked in > > 6.1.1). I am guessing that perhaps the MP tables add entries for each > > socket, but that perhaps the behavior of the shortcuts `-smp n` and > > `-smp cpus=n` was changed to influence the number of cores rather than > > sockets. > > > > In other words, `-smp cpus=n` now means `-smp > > cpus=n,cores=n,threads=1,sockets=1` whereas in 6.1.1 and before it > > meant `-smp cpus=n,cores=1,threads=1,sockets=n`. I note that > > specifying `-smp cpus=4,cores=4,threads=1,sockets=1` in 6.1.1 also > > does not create 4 entries in the legacy MP tables. Thanks for reporting issue in such a detailed way. QEMU doesn't generate legacy MP tables and as reported the above issue is still present in earlier versions when cores are used. Well seabios has a comment: /* Only populate the MPS tables with the first logical CPU in each package */ So I'd guess it has never worked for anything but sockets. With QEMU starting to prefer cores over sockets by default I'd suggest to either * explicitly provide desired topology (i.e. sockets) * use older machine type which still preffers sockets by default (ex: up to 6.1 machine types) If anybody cares about legacy tables + cores/threads usecase, I suggest to investigate what can be done on SeaBIOS side which generates MP tables (assuming if anything could be done at all). CCing SeaBIOS mail-list. > > Can someone confirm/deny this? If so, it's a breaking change that > > perhaps could be mentioned in the Changelog to save others the time > > when they upgrade. Affected educational OS include MIT's xv6 and > > Stanford's pintos OS. Legacy MP table is not actively maintained part of the code, hence it's configuration which is not tested. However if someone is interested in maintaining this, one should contribute at least a testcase that will warn developers early if usecase is broken. We can't promise not breaking it ever but at least we would be able to document any breaking changes in release notes. > > Thanks for all the work you do on qemu! > > > > - Godmar > > > > [1] https://github.com/mit-pdos/xv6-public/blob/eeb7b415dbcb12cc362d0783e41c3d1… > > [2] https://qemu-project.gitlab.io/qemu/about/removed-features.html#smp-invalid… > > > > (I'm typing this email in gmail using the plaintext setting, hopefully > > it'll preserve line breaks.) > > thanks > -- PMM >

6 days

[PATCH] sercon: add missing GET_LOW() to access rx_bytes

by Volker Rümelin

The variable rx_bytes is marked VARLOW. Add a missing GET_LOW() to access rx_bytes. Reported-by: Gabe Black <gabe.black(a)gmail.com> Signed-off-by: Volker Rümelin <vr_qemu(a)t-online.de> --- src/sercon.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sercon.c b/src/sercon.c index 66a1f24..a33dd08 100644 --- a/src/sercon.c +++ b/src/sercon.c @@ -641,7 +641,7 @@ sercon_check_event(void) while (inb(addr + SEROFF_LSR) & 0x01) { byte = inb(addr + SEROFF_DATA); if (GET_LOW(rx_bytes) < sizeof(rx_buf)) { - SET_LOW(rx_buf[rx_bytes], byte); + SET_LOW(rx_buf[GET_LOW(rx_bytes)], byte); SET_LOW(rx_bytes, GET_LOW(rx_bytes) + 1); count++; } -- 2.31.1

6 days, 16 hours

[PATCH] nvme: Move PRP data to ZoneHigh

by Alexander Graf

Commit 01f2736cc905d ("nvme: Pass large I/O requests as PRP lists") introduced multi-page requests using the NVMe PRP mechanism. To store the list and "first page to write to" hints, it added fields to the NVMe namespace struct. Unfortunately, that struct resides in fseg which is read-only at runtime. While KVM ignores the read-only part and allows writes, real hardware and TCG adhere to the semantics and ignore writes to the fseg region. The net effect of that is that reads and writes were always happening on address 0, unless they went through the bounce buffer logic. This patch splits moves all PRP maintenance data from the actual namespace allocation and allocates them in ZoneHigh instead. That way, the PRP list is fully read-write at runtime. Fixes: 01f2736cc905d ("nvme: Pass large I/O requests as PRP lists") Reported-by: Matt DeVillier <matt.devillier(a)gmail.com> Signed-off-by: Alexander Graf <graf(a)amazon.com> --- src/hw/nvme-int.h | 12 ++++++++---- src/hw/nvme.c | 24 ++++++++++++++++-------- 2 files changed, 24 insertions(+), 12 deletions(-) diff --git a/src/hw/nvme-int.h b/src/hw/nvme-int.h index a4c1555..ceb2c79 100644 --- a/src/hw/nvme-int.h +++ b/src/hw/nvme-int.h @@ -108,6 +108,13 @@ struct nvme_ctrl { struct nvme_cq io_cq; }; +/* Page List Maintenance Data */ +struct nvme_prp_info { + u32 prpl_len; + void *prp1; + u64 prpl[NVME_MAX_PRPL_ENTRIES]; +}; + struct nvme_namespace { struct drive_s drive; struct nvme_ctrl *ctrl; @@ -123,10 +130,7 @@ struct nvme_namespace { /* Page aligned buffer of size NVME_PAGE_SIZE. */ char *dma_buffer; - /* Page List */ - u32 prpl_len; - void *prp1; - u64 prpl[NVME_MAX_PRPL_ENTRIES]; + struct nvme_prp_info *prp; }; /* Data structures for NVMe admin identify commands */ diff --git a/src/hw/nvme.c b/src/hw/nvme.c index f035fa2..963c31e 100644 --- a/src/hw/nvme.c +++ b/src/hw/nvme.c @@ -267,6 +267,14 @@ nvme_probe_ns(struct nvme_ctrl *ctrl, u32 ns_idx, u8 mdts) ns->ns_id = ns_id; ns->lba_count = id->nsze; + ns->prp = malloc_high(sizeof(*ns->prp)); + if (!ns->prp) { + warn_noalloc(); + free(ns); + goto free_buffer; + } + memset(ns->prp, 0, sizeof(*ns->prp)); + struct nvme_lba_format *fmt = &id->lbaf[current_lba_format]; ns->block_size = 1U << fmt->lbads; @@ -431,10 +439,10 @@ nvme_io_readwrite(struct nvme_namespace *ns, u64 lba, char *buf, u16 count, if ((ns->block_size * count) > (NVME_PAGE_SIZE * 2)) { /* We need to describe more than 2 pages, rely on PRP List */ - prp2 = ns->prpl; + prp2 = ns->prp->prpl; } else if ((ns->block_size * count) > NVME_PAGE_SIZE) { /* Directly embed the 2nd page if we only need 2 pages */ - prp2 = (void *)(long)ns->prpl[0]; + prp2 = (void *)(long)ns->prp->prpl[0]; } else { /* One page is enough, don't expose anything else */ prp2 = NULL; @@ -465,15 +473,15 @@ nvme_io_readwrite(struct nvme_namespace *ns, u64 lba, char *buf, u16 count, static void nvme_reset_prpl(struct nvme_namespace *ns) { - ns->prpl_len = 0; + ns->prp->prpl_len = 0; } static int nvme_add_prpl(struct nvme_namespace *ns, u64 base) { - if (ns->prpl_len >= NVME_MAX_PRPL_ENTRIES) + if (ns->prp->prpl_len >= NVME_MAX_PRPL_ENTRIES) return -1; - ns->prpl[ns->prpl_len++] = base; + ns->prp->prpl[ns->prp->prpl_len++] = base; return 0; } @@ -492,7 +500,7 @@ static int nvme_build_prpl(struct nvme_namespace *ns, void *op_buf, u16 count) size = count * ns->block_size; /* Special case for transfers that fit into PRP1, but are unaligned */ if (((size + (base & ~NVME_PAGE_MASK)) <= NVME_PAGE_SIZE)) { - ns->prp1 = op_buf; + ns->prp->prp1 = op_buf; return count; } @@ -507,7 +515,7 @@ static int nvme_build_prpl(struct nvme_namespace *ns, void *op_buf, u16 count) for (; size > 0; base += NVME_PAGE_SIZE, size -= NVME_PAGE_SIZE) { if (first_page) { /* First page is special */ - ns->prp1 = (void*)base; + ns->prp->prp1 = (void*)base; first_page = 0; continue; } @@ -726,7 +734,7 @@ nvme_cmd_readwrite(struct nvme_namespace *ns, struct disk_op_s *op, int write) blocks = nvme_build_prpl(ns, op_buf, blocks_remaining); if (blocks) { - res = nvme_io_readwrite(ns, op->lba + i, ns->prp1, blocks, write); + res = nvme_io_readwrite(ns, op->lba + i, ns->prp->prp1, blocks, write); dprintf(5, "ns %u %s lba %llu+%u: %d\n", ns->ns_id, write ? "write" : "read", op->lba, blocks, res); -- 2.28.0.394.ge197136389 Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879

1 week

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

SeaBIOS January 2022