[SeaBIOS] [PATCH v4 3/3] QEMU fw_cfg: Add command to write back address of file
Laszlo Ersek
lersek at redhat.com
Fri Feb 17 12:47:16 CET 2017
On 02/17/17 07:26, ben at skyportsystems.com wrote:
> From: Ben Warren <ben at skyportsystems.com>
>
> This command is similar to ADD_POINTER, but instead of patching
> memory, it writes the pointer back to QEMU over the DMA interface.
>
> Signed-off-by: Ben Warren <ben at skyportsystems.com>
> ---
> src/fw/romfile_loader.c | 40 ++++++++++++++++++++++++++++++++++++++++
> src/fw/romfile_loader.h | 23 ++++++++++++++++++++---
> 2 files changed, 60 insertions(+), 3 deletions(-)
>
> diff --git a/src/fw/romfile_loader.c b/src/fw/romfile_loader.c
> index 7737453..1ddcb2a 100644
> --- a/src/fw/romfile_loader.c
> +++ b/src/fw/romfile_loader.c
> @@ -5,6 +5,7 @@
> #include "romfile.h" // struct romfile_s
> #include "malloc.h" // Zone*, _malloc
> #include "output.h" // warn_*
> +#include "paravirt.h" // qemu_cfg_write_file
>
> struct romfile_loader_file {
> struct romfile_s *file;
> @@ -127,6 +128,41 @@ err:
> warn_internalerror();
> }
>
> +static void romfile_loader_write_pointer(struct romfile_loader_entry_s *entry,
> + struct romfile_loader_files *files)
Whitespace problem, the second line is not correctly aligned.
> +{
> + struct romfile_s *dest_file;
> + struct romfile_loader_file *src_file;
> + unsigned dst_offset = le32_to_cpu(entry->wr_pointer.dst_offset);
> + unsigned src_offset = le32_to_cpu(entry->wr_pointer.src_offset);
> + u64 pointer = 0;
> +
> + /* Writing back to a file that may not be loaded in RAM */
> + dest_file = romfile_find(entry->wr_pointer.dest_file);
> + src_file = romfile_loader_find(entry->wr_pointer.src_file, files);
> +
> + if (!dest_file || !src_file || !src_file->data ||
> + dst_offset + entry->wr_pointer.size < dst_offset ||
> + dst_offset + entry->wr_pointer.size > dest_file->size ||
> + src_offset > src_file->file->size ||
This is off by one (paralleling the earlier assert() off-by-one in QEMU:
<https://www.mail-archive.com/qemu-devel@nongnu.org/msg430195.html>); namely
src_offset == src_file->file->size
would mean that the pointer points one past src_file in memory. While
such pointers are allowed in C (for evaluation, not dereferencing), in
our case it would mean a zero-sized field in src_file for QEMU to
access, and that's wrong. IOW, equality too should be rejected.
> + entry->wr_pointer.size < 1 || entry->wr_pointer.size > 8 ||
> + entry->wr_pointer.size & (entry->wr_pointer.size - 1)) {
> + goto err;
> + }
> +
> + pointer = (unsigned long)src_file->data + src_offset;
This is good, but right after you should range-check the value in
"pointer" against "entry->wr_pointer.size".
There might be a better way for this in SeaBIOS, but the way I do it in
OVMF is like
if ((pointer >> (entry->wr_pointer.size * 8 - 1)) >> 1) {
goto err;
}
The idea is to shift right "entry->wr_pointer.size" bytes out of
"pointer", and see if anything remains (which we won't be able to
represent, so err out then). And the funky double-shift (separating out
the final bit shift) is due to the fact that shifting a uint64_t by 64
bits is undefined behavior, so we shift 63 + 1 bits.
> + pointer = cpu_to_le64(pointer);
> +
> + /* Only supported on QEMU */
> + if (qemu_cfg_write_file(&pointer, dest_file, dst_offset,
> + entry->wr_pointer.size) != entry->wr_pointer.size) {
> + goto err;
> + }
> + return;
> + err:
> + warn_internalerror();
> +}
> +
> int romfile_loader_execute(const char *name)
> {
> struct romfile_loader_entry_s *entry;
> @@ -161,6 +197,10 @@ int romfile_loader_execute(const char *name)
> break;
> case ROMFILE_LOADER_COMMAND_ADD_CHECKSUM:
> romfile_loader_add_checksum(entry, files);
> + break;
> + case ROMFILE_LOADER_COMMAND_WRITE_POINTER:
> + romfile_loader_write_pointer(entry, files);
> + break;
> default:
> /* Skip commands that we don't recognize. */
> break;
> diff --git a/src/fw/romfile_loader.h b/src/fw/romfile_loader.h
> index bce3719..8b557a3 100644
> --- a/src/fw/romfile_loader.h
> +++ b/src/fw/romfile_loader.h
> @@ -51,15 +51,32 @@ struct romfile_loader_entry_s {
> u32 length;
> } cksum;
>
> + /*
> + * COMMAND_WRITE_POINTER - Write back to a host file via DMA,
> + * wr_pointer at dest_file at offset @wr_pointer.dst_offset, a pointer
"wr_pointer at dest_file" should be "@wr_pointer.dest_file".
The rest looks good.
Thanks!
> + * to the table originating from @wr_pointer.src_file at offset
> + * @wr_pointer.src_offset.
> + * 1,2,4 or 8 byte unsigned addition is used depending on
> + * @wr_pointer.size.
> + */
> + struct {
> + char dest_file[ROMFILE_LOADER_FILESZ];
> + char src_file[ROMFILE_LOADER_FILESZ];
> + u32 dst_offset;
> + u32 src_offset;
> + u8 size;
> + } wr_pointer;
> +
> /* padding */
> char pad[124];
> };
> };
>
> enum {
> - ROMFILE_LOADER_COMMAND_ALLOCATE = 0x1,
> - ROMFILE_LOADER_COMMAND_ADD_POINTER = 0x2,
> - ROMFILE_LOADER_COMMAND_ADD_CHECKSUM = 0x3,
> + ROMFILE_LOADER_COMMAND_ALLOCATE = 0x1,
> + ROMFILE_LOADER_COMMAND_ADD_POINTER = 0x2,
> + ROMFILE_LOADER_COMMAND_ADD_CHECKSUM = 0x3,
> + ROMFILE_LOADER_COMMAND_WRITE_POINTER = 0x4,
> };
>
> enum {
>
More information about the SeaBIOS
mailing list