From 8f8c04bf1ebbd2f72f1643e7ad9617dafa6e5409 Mon Sep 17 00:00:00 2001 From: Nicolas Iooss Date: Fri, 10 Jun 2022 14:50:25 +0000 Subject: [PATCH 1/6] i2c: fix stack buffer overflow vulnerability in i2c md command When running "i2c md 0 0 80000100", the function do_i2c_md parses the length into an unsigned int variable named length. The value is then moved to a signed variable: int nbytes = length; #define DISP_LINE_LEN 16 int linebytes = (nbytes > DISP_LINE_LEN) ? DISP_LINE_LEN : nbytes; ret = dm_i2c_read(dev, addr, linebuf, linebytes); On systems where integers are 32 bits wide, 0x80000100 is a negative value to "nbytes > DISP_LINE_LEN" is false and linebytes gets assigned 0x80000100 instead of 16. The consequence is that the function which reads from the i2c device (dm_i2c_read or i2c_read) is called with a 16-byte stack buffer to fill but with a size parameter which is too large. In some cases, this could trigger a crash. But with some i2c drivers, such as drivers/i2c/nx_i2c.c (used with "nexell,s5pxx18-i2c" bus), the size is actually truncated to a 16-bit integer. This is because function i2c_transfer expects an unsigned short length. In such a case, an attacker who can control the response of an i2c device can overwrite the return address of a function and execute arbitrary code through Return-Oriented Programming. Fix this issue by using unsigned integers types in do_i2c_md. While at it, make also alen unsigned, as signed sizes can cause vulnerabilities when people forgot to check that they can be negative. Signed-off-by: Nicolas Iooss Reviewed-by: Heiko Schocher --- cmd/i2c.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/cmd/i2c.c b/cmd/i2c.c index 9050b2b8d2..bd04b14024 100644 --- a/cmd/i2c.c +++ b/cmd/i2c.c @@ -200,10 +200,10 @@ void i2c_init_board(void) * * Returns the address length. */ -static uint get_alen(char *arg, int default_len) +static uint get_alen(char *arg, uint default_len) { - int j; - int alen; + uint j; + uint alen; alen = default_len; for (j = 0; j < 8; j++) { @@ -247,7 +247,7 @@ static int do_i2c_read(struct cmd_tbl *cmdtp, int flag, int argc, { uint chip; uint devaddr, length; - int alen; + uint alen; u_char *memaddr; int ret; #if CONFIG_IS_ENABLED(DM_I2C) @@ -301,7 +301,7 @@ static int do_i2c_write(struct cmd_tbl *cmdtp, int flag, int argc, { uint chip; uint devaddr, length; - int alen; + uint alen; u_char *memaddr; int ret; #if CONFIG_IS_ENABLED(DM_I2C) @@ -469,8 +469,8 @@ static int do_i2c_md(struct cmd_tbl *cmdtp, int flag, int argc, { uint chip; uint addr, length; - int alen; - int j, nbytes, linebytes; + uint alen; + uint j, nbytes, linebytes; int ret; #if CONFIG_IS_ENABLED(DM_I2C) struct udevice *dev; @@ -589,9 +589,9 @@ static int do_i2c_mw(struct cmd_tbl *cmdtp, int flag, int argc, { uint chip; ulong addr; - int alen; + uint alen; uchar byte; - int count; + uint count; int ret; #if CONFIG_IS_ENABLED(DM_I2C) struct udevice *dev; @@ -676,8 +676,8 @@ static int do_i2c_crc(struct cmd_tbl *cmdtp, int flag, int argc, { uint chip; ulong addr; - int alen; - int count; + uint alen; + uint count; uchar byte; ulong crc; ulong err; @@ -985,7 +985,7 @@ static int do_i2c_loop(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { uint chip; - int alen; + uint alen; uint addr; uint length; u_char bytes[16]; From 7f7fb9937c6cb49dd35153bd6708872b390b0a44 Mon Sep 17 00:00:00 2001 From: Miquel Raynal Date: Mon, 27 Jun 2022 12:20:03 +0200 Subject: [PATCH 2/6] fs/squashfs: Use kcalloc when relevant A crafted squashfs image could embed a huge number of empty metadata blocks in order to make the amount of malloc()'d memory overflow and be much smaller than expected. Because of this flaw, any random code positioned at the right location in the squashfs image could be memcpy'd from the squashfs structures into U-Boot code location while trying to access the rearmost blocks, before being executed. In order to prevent this vulnerability from being exploited in eg. a secure boot environment, let's add a check over the amount of data that is going to be allocated. Such a check could look like: if (!elem_size || n > SIZE_MAX / elem_size) return NULL; The right way to do it would be to enhance the calloc() implementation but this is quite an impacting change for such a small fix. Another solution would be to add the check before the malloc call in the squashfs implementation, but this does not look right. So for now, let's use the kcalloc() compatibility function from Linux, which has this check. Fixes: c5100613037 ("fs/squashfs: new filesystem") Reported-by: Tatsuhiko Yasumatsu Signed-off-by: Miquel Raynal Tested-by: Tatsuhiko Yasumatsu --- fs/squashfs/sqfs.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index b9f05efd9c..9c1f87caca 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -13,6 +13,7 @@ #include #include #include +#include #include #include #include @@ -725,7 +726,8 @@ static int sqfs_read_inode_table(unsigned char **inode_table) goto free_itb; } - *inode_table = malloc(metablks_count * SQFS_METADATA_BLOCK_SIZE); + *inode_table = kcalloc(metablks_count, SQFS_METADATA_BLOCK_SIZE, + GFP_KERNEL); if (!*inode_table) { ret = -ENOMEM; printf("Error: failed to allocate squashfs inode_table of size %i, increasing CONFIG_SYS_MALLOC_LEN could help\n", From 93899d0746f6fe464d6210b0554e917f60444304 Mon Sep 17 00:00:00 2001 From: Dario Binacchi Date: Sun, 26 Jun 2022 12:05:15 +0200 Subject: [PATCH 3/6] configs: imx8mn_bsh_smm_s2: add NAND driver It allows to boot from NAND. Co-developed-by: Michael Trimarchi Signed-off-by: Michael Trimarchi Signed-off-by: Dario Binacchi --- configs/imx8mn_bsh_smm_s2_defconfig | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/configs/imx8mn_bsh_smm_s2_defconfig b/configs/imx8mn_bsh_smm_s2_defconfig index 49f4253001..08f52e5060 100644 --- a/configs/imx8mn_bsh_smm_s2_defconfig +++ b/configs/imx8mn_bsh_smm_s2_defconfig @@ -30,8 +30,10 @@ CONFIG_SPL_BOARD_INIT=y CONFIG_SPL_BOOTROM_SUPPORT=y CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR=y CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR=0x300 +CONFIG_SPL_DMA=y CONFIG_SPL_I2C=y CONFIG_SPL_MTD_SUPPORT=y +CONFIG_SPL_NAND_SUPPORT=y CONFIG_SPL_POWER=y CONFIG_SPL_WATCHDOG=y CONFIG_SYS_PROMPT="> " @@ -65,6 +67,9 @@ CONFIG_SYS_NAND_USE_FLASH_BBT=y CONFIG_NAND_MXS=y CONFIG_NAND_MXS_DT=y CONFIG_SYS_NAND_ONFI_DETECTION=y +CONFIG_SYS_NAND_U_BOOT_LOCATIONS=y +CONFIG_SYS_NAND_U_BOOT_OFFS=0xD8000 +CONFIG_SYS_NAND_U_BOOT_OFFS_REDUND=0x4058000 CONFIG_PHYLIB=y CONFIG_PHY_NXP_TJA11XX=y CONFIG_DM_ETH=y From 592e04cde563425ef9d92b2a4b4927906dbeecd6 Mon Sep 17 00:00:00 2001 From: Dario Binacchi Date: Sun, 26 Jun 2022 12:05:16 +0200 Subject: [PATCH 4/6] configs: imx8mn_bsh_smm_s2: add UBI commands imx8mn_bsh_smm_s2 uses ubifs rootfs, UBI commands are required to flash it. Co-developed-by: Michael Trimarchi Signed-off-by: Michael Trimarchi Signed-off-by: Dario Binacchi --- configs/imx8mn_bsh_smm_s2_defconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/configs/imx8mn_bsh_smm_s2_defconfig b/configs/imx8mn_bsh_smm_s2_defconfig index 08f52e5060..f8c75a2b23 100644 --- a/configs/imx8mn_bsh_smm_s2_defconfig +++ b/configs/imx8mn_bsh_smm_s2_defconfig @@ -43,6 +43,7 @@ CONFIG_CMD_USB_MASS_STORAGE=y CONFIG_CMD_MTDPARTS=y CONFIG_MTDIDS_DEFAULT="nand0=gpmi-nand" CONFIG_MTDPARTS_DEFAULT="gpmi-nand:64m(nandboot),16m(nandfit),32m(nandkernel),1m(nanddtb),8m(nandtee),-(nandrootfs)" +CONFIG_CMD_UBI=y CONFIG_OF_CONTROL=y CONFIG_SPL_OF_CONTROL=y CONFIG_SYS_RELOC_GD_ENV_ADDR=y From bede82f7507523f23390afe7c373be6c540c5431 Mon Sep 17 00:00:00 2001 From: Dario Binacchi Date: Sun, 26 Jun 2022 12:05:17 +0200 Subject: [PATCH 5/6] configs: imx8mn_bsh_smm_s2: remove console from bootargs The Linux kernel device tree already specifies the device to be used for boot console output with a stdout-path property under /chosen. Co-developed-by: Michael Trimarchi Signed-off-by: Michael Trimarchi Signed-off-by: Dario Binacchi --- include/configs/imx8mn_bsh_smm_s2.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/configs/imx8mn_bsh_smm_s2.h b/include/configs/imx8mn_bsh_smm_s2.h index 098f23b206..1eff8c4370 100644 --- a/include/configs/imx8mn_bsh_smm_s2.h +++ b/include/configs/imx8mn_bsh_smm_s2.h @@ -16,7 +16,7 @@ #define NANDARGS \ "mtdids=" CONFIG_MTDIDS_DEFAULT "\0" \ "mtdparts=" CONFIG_MTDPARTS_DEFAULT "\0" \ - "nandargs=setenv bootargs console=${console} " \ + "nandargs=setenv bootargs " \ "${optargs} " \ "root=${nandroot} " \ "rootfstype=${nandrootfstype}\0" \ From 0c4db621e265c780833af4152a99398aed482f61 Mon Sep 17 00:00:00 2001 From: Dario Binacchi Date: Sun, 26 Jun 2022 12:05:18 +0200 Subject: [PATCH 6/6] configs: imx8mn_bsh_smm_s2: add mtdparts to bootargs Passing the mtdparts environment variable to the Linux kernel is required to properly mount the UBI rootfs. Co-developed-by: Michael Trimarchi Signed-off-by: Michael Trimarchi Signed-off-by: Dario Binacchi --- include/configs/imx8mn_bsh_smm_s2.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/configs/imx8mn_bsh_smm_s2.h b/include/configs/imx8mn_bsh_smm_s2.h index 1eff8c4370..d09c2ab016 100644 --- a/include/configs/imx8mn_bsh_smm_s2.h +++ b/include/configs/imx8mn_bsh_smm_s2.h @@ -18,6 +18,7 @@ "mtdparts=" CONFIG_MTDPARTS_DEFAULT "\0" \ "nandargs=setenv bootargs " \ "${optargs} " \ + "mtdparts=${mtdparts} " \ "root=${nandroot} " \ "rootfstype=${nandrootfstype}\0" \ "nandroot=ubi0:root rw ubi.mtd=nandrootfs\0" \