| Message ID | 20250203105912.196654-6-sughosh.ganu@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Add pmem node for preserving distro ISO's | expand |
Hi Sughosh This generally looks ok, but I don't love the idea of unconditionally preserving all slices regardless of their usage. Basically, if a user doesn't unmap that slice it will end in kernel memory. My fear is that someone will forget device sensitive data in a blkmap.... On Mon, 3 Feb 2025 at 12:59, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > The EFI HTTP boot puts the ISO installer image at some location in > memory which needs to be added to the devicetree as persistent > memory (pmem) node. The OS installer then gets information about the > presence of this ISO image through the pmem node and proceeds with the > installation. > > In U-Boot, this ISO image gets mounted as a blkmap device, with a > memory mapped slice. Add a helper function which iterates through all > such memory mapped blkmap slices, and calls the FDT fixup function to > add the pmem node. Invoke this helper function as part of the DT fixup > which happens before booting the OS. > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > --- > Changes since V3: > * Move the definition of the helper function to the efi_helper.c > * Remove the region of the blkmap mem map device from the EFI memory > map along with adding the pmem node > [...] > @@ -680,3 +683,52 @@ out: > > return ret; > } > + > +static int add_blkmap_pmem_nodes(void *fdt, struct blkmap *bm) > +{ > + int ret; > + u32 size; > + ulong addr; > + efi_status_t status; > + struct blkmap_mem *bmm; > + struct blkmap_slice *bms; > + struct blk_desc *bd = dev_get_uclass_plat(bm->blk); > + > + list_for_each_entry(bms, &bm->slices, node) { > + if (bms->type != BLKMAP_SLICE_MEM) > + continue; Can we convert the 'type' to 'preserve' and teach blkmap_create_ramdisk() to pass that flag? This way we can unconditionally pass it from EFI HTTP installers, and let the command line users decide if they want to preserve it. > + > + bmm = container_of(bms, struct blkmap_mem, slice); > + > + addr = (ulong)(uintptr_t)bmm->addr; > + size = (u32)bms->blkcnt << bd->log2blksz; > + > + ret = fdt_fixup_pmem_region(fdt, addr, size); > + if (ret) > + return ret; > + > + status = efi_remove_memory_map(addr, size, > + EFI_CONVENTIONAL_MEMORY); > + if (status != EFI_SUCCESS) > + return -1; > + } > + > + return 0; > +} > + Thanks /Ilias
Am 21. Februar 2025 19:55:03 MEZ schrieb Ilias Apalodimas <ilias.apalodimas@linaro.org>: >Hi Sughosh > >This generally looks ok, but I don't love the idea of unconditionally >preserving all slices regardless of their usage. >Basically, if a user doesn't unmap that slice it will end in kernel >memory. My fear is that someone will forget device sensitive data in a >blkmap.... I don't that you can easily determine which blkmap and which blkmap slice was created why. But can we realisticly assume that a device that U-Boot reads from is secure against reading at kernel runtime. I don't believe so as U-Boot runs at the same exception level as the kernel. U-Boot and the kernel do not empty RAM. You always have to assume that whatever is in U-Boot memory is dicoverable at kernel runtime. If you load sensitive data, you must overwrite it before booting. Best regards Heinrich > >On Mon, 3 Feb 2025 at 12:59, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: >> >> The EFI HTTP boot puts the ISO installer image at some location in >> memory which needs to be added to the devicetree as persistent >> memory (pmem) node. The OS installer then gets information about the >> presence of this ISO image through the pmem node and proceeds with the >> installation. >> >> In U-Boot, this ISO image gets mounted as a blkmap device, with a >> memory mapped slice. Add a helper function which iterates through all >> such memory mapped blkmap slices, and calls the FDT fixup function to >> add the pmem node. Invoke this helper function as part of the DT fixup >> which happens before booting the OS. >> >> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> >> --- >> Changes since V3: >> * Move the definition of the helper function to the efi_helper.c >> * Remove the region of the blkmap mem map device from the EFI memory >> map along with adding the pmem node >> > >[...] > >> @@ -680,3 +683,52 @@ out: >> >> return ret; >> } >> + >> +static int add_blkmap_pmem_nodes(void *fdt, struct blkmap *bm) >> +{ >> + int ret; >> + u32 size; >> + ulong addr; >> + efi_status_t status; >> + struct blkmap_mem *bmm; >> + struct blkmap_slice *bms; >> + struct blk_desc *bd = dev_get_uclass_plat(bm->blk); >> + >> + list_for_each_entry(bms, &bm->slices, node) { >> + if (bms->type != BLKMAP_SLICE_MEM) >> + continue; > >Can we convert the 'type' to 'preserve' and teach >blkmap_create_ramdisk() to pass that flag? >This way we can unconditionally pass it from EFI HTTP installers, and >let the command line users decide if they want to preserve it. > > >> + >> + bmm = container_of(bms, struct blkmap_mem, slice); >> + >> + addr = (ulong)(uintptr_t)bmm->addr; >> + size = (u32)bms->blkcnt << bd->log2blksz; >> + >> + ret = fdt_fixup_pmem_region(fdt, addr, size); >> + if (ret) >> + return ret; >> + >> + status = efi_remove_memory_map(addr, size, >> + EFI_CONVENTIONAL_MEMORY); >> + if (status != EFI_SUCCESS) >> + return -1; >> + } >> + >> + return 0; >> +} >> + > > >Thanks >/Ilias
On Fri, 21 Feb 2025 at 21:22, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > Am 21. Februar 2025 19:55:03 MEZ schrieb Ilias Apalodimas <ilias.apalodimas@linaro.org>: > >Hi Sughosh > > > >This generally looks ok, but I don't love the idea of unconditionally > >preserving all slices regardless of their usage. > >Basically, if a user doesn't unmap that slice it will end in kernel > >memory. My fear is that someone will forget device sensitive data in a > >blkmap.... > > I don't that you can easily determine which blkmap and which blkmap slice was created why. > > But can we realisticly assume that a device that U-Boot reads from is secure against reading at kernel runtime. I don't believe so as U-Boot runs at the same exception level as the kernel. U-Boot and the kernel do not empty RAM. You always have to assume that whatever is in U-Boot memory is dicoverable at kernel runtime. > > If you load sensitive data, you must overwrite it before booting. Fair enough. I am not against this patch, I am just to figure out if we can restrict what we preserve a bit more Thanks /Ilias > > Best regards > > Heinrich > > > > > > >On Mon, 3 Feb 2025 at 12:59, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > >> > >> The EFI HTTP boot puts the ISO installer image at some location in > >> memory which needs to be added to the devicetree as persistent > >> memory (pmem) node. The OS installer then gets information about the > >> presence of this ISO image through the pmem node and proceeds with the > >> installation. > >> > >> In U-Boot, this ISO image gets mounted as a blkmap device, with a > >> memory mapped slice. Add a helper function which iterates through all > >> such memory mapped blkmap slices, and calls the FDT fixup function to > >> add the pmem node. Invoke this helper function as part of the DT fixup > >> which happens before booting the OS. > >> > >> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > >> --- > >> Changes since V3: > >> * Move the definition of the helper function to the efi_helper.c > >> * Remove the region of the blkmap mem map device from the EFI memory > >> map along with adding the pmem node > >> > > > >[...] > > > >> @@ -680,3 +683,52 @@ out: > >> > >> return ret; > >> } > >> + > >> +static int add_blkmap_pmem_nodes(void *fdt, struct blkmap *bm) > >> +{ > >> + int ret; > >> + u32 size; > >> + ulong addr; > >> + efi_status_t status; > >> + struct blkmap_mem *bmm; > >> + struct blkmap_slice *bms; > >> + struct blk_desc *bd = dev_get_uclass_plat(bm->blk); > >> + > >> + list_for_each_entry(bms, &bm->slices, node) { > >> + if (bms->type != BLKMAP_SLICE_MEM) > >> + continue; > > > >Can we convert the 'type' to 'preserve' and teach > >blkmap_create_ramdisk() to pass that flag? > >This way we can unconditionally pass it from EFI HTTP installers, and > >let the command line users decide if they want to preserve it. > > > > > >> + > >> + bmm = container_of(bms, struct blkmap_mem, slice); > >> + > >> + addr = (ulong)(uintptr_t)bmm->addr; > >> + size = (u32)bms->blkcnt << bd->log2blksz; > >> + > >> + ret = fdt_fixup_pmem_region(fdt, addr, size); > >> + if (ret) > >> + return ret; > >> + > >> + status = efi_remove_memory_map(addr, size, > >> + EFI_CONVENTIONAL_MEMORY); > >> + if (status != EFI_SUCCESS) > >> + return -1; > >> + } > >> + > >> + return 0; > >> +} > >> + > > > > > >Thanks > >/Ilias >
On fre, feb 21, 2025 at 20:55, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > Hi Sughosh > > This generally looks ok, but I don't love the idea of unconditionally > preserving all slices regardless of their usage. > Basically, if a user doesn't unmap that slice it will end in kernel > memory. My fear is that someone will forget device sensitive data in a > blkmap.... > > On Mon, 3 Feb 2025 at 12:59, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: >> >> The EFI HTTP boot puts the ISO installer image at some location in >> memory which needs to be added to the devicetree as persistent >> memory (pmem) node. The OS installer then gets information about the >> presence of this ISO image through the pmem node and proceeds with the >> installation. >> >> In U-Boot, this ISO image gets mounted as a blkmap device, with a >> memory mapped slice. Add a helper function which iterates through all >> such memory mapped blkmap slices, and calls the FDT fixup function to >> add the pmem node. Invoke this helper function as part of the DT fixup >> which happens before booting the OS. >> >> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> >> --- >> Changes since V3: >> * Move the definition of the helper function to the efi_helper.c >> * Remove the region of the blkmap mem map device from the EFI memory >> map along with adding the pmem node >> > > [...] > >> @@ -680,3 +683,52 @@ out: >> >> return ret; >> } >> + >> +static int add_blkmap_pmem_nodes(void *fdt, struct blkmap *bm) >> +{ >> + int ret; >> + u32 size; >> + ulong addr; >> + efi_status_t status; >> + struct blkmap_mem *bmm; >> + struct blkmap_slice *bms; >> + struct blk_desc *bd = dev_get_uclass_plat(bm->blk); >> + >> + list_for_each_entry(bms, &bm->slices, node) { >> + if (bms->type != BLKMAP_SLICE_MEM) >> + continue; > > Can we convert the 'type' to 'preserve' and teach > blkmap_create_ramdisk() to pass that flag? > This way we can unconditionally pass it from EFI HTTP installers, and > let the command line users decide if they want to preserve it. This seems like the most reasonable approach to me as well. Then we could add a single API like this: int blkmap_foreach_pmem_slice(int (*cb)(void *ctx, void *addr, size_t size), void *ctx); Rather than exporting all internal details of every slice's implementation in blkmap.h. I.e., let the blkmap code deal with how to locate the slices of interest, and keep the internal details away from the consumer of the data. With that added to blkmap.c, I think the rest of this patch reduces to something like: int add_pmem_node(void *fdt, void *addr, size_t size) { return fdt_fixup_pmem_region(fdt, (ulong)addr, size); } int fdt_efi_pmem_setup(void *fdt) { return blkmap_foreach_pmem_slice(add_pmem_node, fdt); } > >> + >> + bmm = container_of(bms, struct blkmap_mem, slice); >> + >> + addr = (ulong)(uintptr_t)bmm->addr; >> + size = (u32)bms->blkcnt << bd->log2blksz; >> + >> + ret = fdt_fixup_pmem_region(fdt, addr, size); >> + if (ret) >> + return ret; >> + >> + status = efi_remove_memory_map(addr, size, >> + EFI_CONVENTIONAL_MEMORY); >> + if (status != EFI_SUCCESS) >> + return -1; >> + } >> + >> + return 0; >> +} >> + > > > Thanks > /Ilias
diff --git a/boot/image-fdt.c b/boot/image-fdt.c index 9d1598b1a93..e64243af88e 100644 --- a/boot/image-fdt.c +++ b/boot/image-fdt.c @@ -8,6 +8,7 @@ * Wolfgang Denk, DENX Software Engineering, wd@denx.de. */ +#include <blkmap.h> #include <command.h> #include <fdt_support.h> #include <fdtdec.h> @@ -649,6 +650,14 @@ int image_setup_libfdt(struct bootm_headers *images, void *blob, bool lmb) if (!ft_verify_fdt(blob)) goto err; + if (CONFIG_IS_ENABLED(BLKMAP) && CONFIG_IS_ENABLED(EFI_LOADER)) { + fdt_ret = fdt_efi_pmem_setup(blob); + if (fdt_ret) { + log_err("pmem node fixup failed\n"); + goto err; + } + } + /* after here we are using a livetree */ if (!of_live_active() && CONFIG_IS_ENABLED(EVENT)) { struct event_ft_fixup fixup; diff --git a/drivers/block/blkmap.c b/drivers/block/blkmap.c index 4c71ec784e0..2c7f4966ae9 100644 --- a/drivers/block/blkmap.c +++ b/drivers/block/blkmap.c @@ -14,59 +14,6 @@ #include <dm/lists.h> #include <dm/root.h> -struct blkmap; - -/** - * struct blkmap_slice - Region mapped to a blkmap - * - * Common data for a region mapped to a blkmap, specialized by each - * map type. - * - * @node: List node used to associate this slice with a blkmap - * @blknr: Start block number of the mapping - * @blkcnt: Number of blocks covered by this mapping - * @type: Type of blkmap slice - */ -struct blkmap_slice { - struct list_head node; - - lbaint_t blknr; - lbaint_t blkcnt; - enum blkmap_slice_type type; - - /** - * @read: - Read from slice - * - * @read.bm: Blkmap to which this slice belongs - * @read.bms: This slice - * @read.blknr: Start block number to read from - * @read.blkcnt: Number of blocks to read - * @read.buffer: Buffer to store read data to - */ - ulong (*read)(struct blkmap *bm, struct blkmap_slice *bms, - lbaint_t blknr, lbaint_t blkcnt, void *buffer); - - /** - * @write: - Write to slice - * - * @write.bm: Blkmap to which this slice belongs - * @write.bms: This slice - * @write.blknr: Start block number to write to - * @write.blkcnt: Number of blocks to write - * @write.buffer: Data to be written - */ - ulong (*write)(struct blkmap *bm, struct blkmap_slice *bms, - lbaint_t blknr, lbaint_t blkcnt, const void *buffer); - - /** - * @destroy: - Tear down slice - * - * @read.bm: Blkmap to which this slice belongs - * @read.bms: This slice - */ - void (*destroy)(struct blkmap *bm, struct blkmap_slice *bms); -}; - static bool blkmap_slice_contains(struct blkmap_slice *bms, lbaint_t blknr) { return (blknr >= bms->blknr) && (blknr < (bms->blknr + bms->blkcnt)); @@ -116,20 +63,6 @@ static int blkmap_slice_add(struct blkmap *bm, struct blkmap_slice *new) return 0; } -/** - * struct blkmap_linear - Linear mapping to other block device - * - * @slice: Common map data - * @blk: Target block device of this mapping - * @blknr: Start block number of the target device - */ -struct blkmap_linear { - struct blkmap_slice slice; - - struct udevice *blk; - lbaint_t blknr; -}; - static ulong blkmap_linear_read(struct blkmap *bm, struct blkmap_slice *bms, lbaint_t blknr, lbaint_t blkcnt, void *buffer) { @@ -188,21 +121,6 @@ int blkmap_map_linear(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt, return err; } -/** - * struct blkmap_mem - Memory mapping - * - * @slice: Common map data - * @addr: Target memory region of this mapping - * @remapped: True if @addr is backed by a physical to virtual memory - * mapping that must be torn down at the end of this mapping's - * lifetime. - */ -struct blkmap_mem { - struct blkmap_slice slice; - void *addr; - bool remapped; -}; - static ulong blkmap_mem_read(struct blkmap *bm, struct blkmap_slice *bms, lbaint_t blknr, lbaint_t blkcnt, void *buffer) { diff --git a/include/blkmap.h b/include/blkmap.h index c7b4bf13c4e..e5e2d971548 100644 --- a/include/blkmap.h +++ b/include/blkmap.h @@ -7,6 +7,7 @@ #ifndef _BLKMAP_H #define _BLKMAP_H +#include <blk.h> #include <dm/lists.h> /* Type of blkmap slice, Linear or Memory */ @@ -30,6 +31,86 @@ struct blkmap { struct list_head slices; }; +/** + * struct blkmap_slice - Region mapped to a blkmap + * + * Common data for a region mapped to a blkmap, specialized by each + * map type. + * + * @node: List node used to associate this slice with a blkmap + * @blknr: Start block number of the mapping + * @blkcnt: Number of blocks covered by this mapping + * @type: Type of blkmap slice + */ +struct blkmap_slice { + struct list_head node; + + lbaint_t blknr; + lbaint_t blkcnt; + enum blkmap_slice_type type; + + /** + * @read: - Read from slice + * + * @read.bm: Blkmap to which this slice belongs + * @read.bms: This slice + * @read.blknr: Start block number to read from + * @read.blkcnt: Number of blocks to read + * @read.buffer: Buffer to store read data to + */ + ulong (*read)(struct blkmap *bm, struct blkmap_slice *bms, + lbaint_t blknr, lbaint_t blkcnt, void *buffer); + + /** + * @write: - Write to slice + * + * @write.bm: Blkmap to which this slice belongs + * @write.bms: This slice + * @write.blknr: Start block number to write to + * @write.blkcnt: Number of blocks to write + * @write.buffer: Data to be written + */ + ulong (*write)(struct blkmap *bm, struct blkmap_slice *bms, + lbaint_t blknr, lbaint_t blkcnt, const void *buffer); + + /** + * @destroy: - Tear down slice + * + * @read.bm: Blkmap to which this slice belongs + * @read.bms: This slice + */ + void (*destroy)(struct blkmap *bm, struct blkmap_slice *bms); +}; + +/** + * struct blkmap_mem - Memory mapping + * + * @slice: Common map data + * @addr: Target memory region of this mapping + * @remapped: True if @addr is backed by a physical to virtual memory + * mapping that must be torn down at the end of this mapping's + * lifetime. + */ +struct blkmap_mem { + struct blkmap_slice slice; + void *addr; + bool remapped; +}; + +/** + * struct blkmap_linear - Linear mapping to other block device + * + * @slice: Common map data + * @blk: Target block device of this mapping + * @blknr: Start block number of the target device + */ +struct blkmap_linear { + struct blkmap_slice slice; + + struct udevice *blk; + lbaint_t blknr; +}; + /** * blkmap_map_linear() - Map region of other block device * diff --git a/include/efi.h b/include/efi.h index d005cb6181e..f9bbb175c3a 100644 --- a/include/efi.h +++ b/include/efi.h @@ -705,4 +705,17 @@ static inline bool efi_use_host_arch(void) */ int efi_get_pxe_arch(void); +/** + * fdt_efi_pmem_setup() - Pmem setup in DT and EFI memory map + * @fdt: Devicetree to add the pmem nodes to + * + * Iterate through all the blkmap devices, look for BLKMAP_MEM devices, + * and add pmem nodes corresponding to the blkmap slice to the + * devicetree along with removing the corresponding region from the + * EFI memory map. + * + * Returns: 0 on success, negative error on failure + */ +int fdt_efi_pmem_setup(void *fdt); + #endif /* _LINUX_EFI_H */ diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c index 04b2efc4a3b..1050ed1f952 100644 --- a/lib/efi_loader/efi_helper.c +++ b/lib/efi_loader/efi_helper.c @@ -5,6 +5,7 @@ #define LOG_CATEGORY LOGC_EFI +#include <blkmap.h> #include <bootm.h> #include <env.h> #include <image.h> @@ -19,6 +20,8 @@ #include <efi_loader.h> #include <efi_variable.h> #include <host_arch.h> +#include <dm/uclass.h> +#include <linux/kernel.h> #include <linux/libfdt.h> #include <linux/list.h> @@ -680,3 +683,52 @@ out: return ret; } + +static int add_blkmap_pmem_nodes(void *fdt, struct blkmap *bm) +{ + int ret; + u32 size; + ulong addr; + efi_status_t status; + struct blkmap_mem *bmm; + struct blkmap_slice *bms; + struct blk_desc *bd = dev_get_uclass_plat(bm->blk); + + list_for_each_entry(bms, &bm->slices, node) { + if (bms->type != BLKMAP_SLICE_MEM) + continue; + + bmm = container_of(bms, struct blkmap_mem, slice); + + addr = (ulong)(uintptr_t)bmm->addr; + size = (u32)bms->blkcnt << bd->log2blksz; + + ret = fdt_fixup_pmem_region(fdt, addr, size); + if (ret) + return ret; + + status = efi_remove_memory_map(addr, size, + EFI_CONVENTIONAL_MEMORY); + if (status != EFI_SUCCESS) + return -1; + } + + return 0; +} + +int fdt_efi_pmem_setup(void *fdt) +{ + int ret; + struct udevice *dev; + struct uclass *uc; + struct blkmap *bm; + + uclass_id_foreach_dev(UCLASS_BLKMAP, dev, uc) { + bm = dev_get_plat(dev); + ret = add_blkmap_pmem_nodes(fdt, bm); + if (ret) + return ret; + } + + return 0; +}
The EFI HTTP boot puts the ISO installer image at some location in memory which needs to be added to the devicetree as persistent memory (pmem) node. The OS installer then gets information about the presence of this ISO image through the pmem node and proceeds with the installation. In U-Boot, this ISO image gets mounted as a blkmap device, with a memory mapped slice. Add a helper function which iterates through all such memory mapped blkmap slices, and calls the FDT fixup function to add the pmem node. Invoke this helper function as part of the DT fixup which happens before booting the OS. Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> --- Changes since V3: * Move the definition of the helper function to the efi_helper.c * Remove the region of the blkmap mem map device from the EFI memory map along with adding the pmem node boot/image-fdt.c | 9 ++++ drivers/block/blkmap.c | 82 ------------------------------------- include/blkmap.h | 81 ++++++++++++++++++++++++++++++++++++ include/efi.h | 13 ++++++ lib/efi_loader/efi_helper.c | 52 +++++++++++++++++++++++ 5 files changed, 155 insertions(+), 82 deletions(-)