| Message ID | 20230813083912.532982-1-loic.poulain@linaro.org |
|---|---|
| State | New |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | usb: gadget: sdp: Option to enable SDP read register command | expand |
On 8/13/23 10:39, Loic Poulain wrote: > The SDP read register command can be used to read any memory > mapped address of the device (ddr, registers...). It can then > be exploited by an attacker to access sensitive data/values, > especially when running SDP from SPL, as SPL runs with highest > privileges in ARM secure mode. > > Without read, SDP still useful to bootstrap and jump on (signed) > blob such as u-boot with write and jump commands, but reading > is optional in that case (debug purpose). > > NXP SoCs usually have a dedicated SDP_READ_DISABLE fuse to disable > SDP read command in their ROM SDP implementation, so it seems quite > reasonable to make it optional from u-boot/spl as well. If there is a fuse, why not read the fuse and disable READ based on that fuse instead ?
Hi Marek, On Mon, 14 Aug 2023 at 01:53, Marek Vasut <marex@denx.de> wrote: > > On 8/13/23 10:39, Loic Poulain wrote: > > The SDP read register command can be used to read any memory > > mapped address of the device (ddr, registers...). It can then > > be exploited by an attacker to access sensitive data/values, > > especially when running SDP from SPL, as SPL runs with highest > > privileges in ARM secure mode. > > > > Without read, SDP still useful to bootstrap and jump on (signed) > > blob such as u-boot with write and jump commands, but reading > > is optional in that case (debug purpose). > > > > NXP SoCs usually have a dedicated SDP_READ_DISABLE fuse to disable > > SDP read command in their ROM SDP implementation, so it seems quite > > reasonable to make it optional from u-boot/spl as well. > > If there is a fuse, why not read the fuse and disable READ based on that > fuse instead ? Well, fuse is more a way to tune a specific ROM code here, not the software. It would be more generic to make it a build config like other features, and one may purposely force SDP READ in SPL, even if disabled at ROM level. That said we could also introduce a weak board_sdp_read_allowed() function... Let me know what you prefer. Regards, Loic
On 9/1/23 12:03, Loic Poulain wrote: > Hi Marek, > > On Mon, 14 Aug 2023 at 01:53, Marek Vasut <marex@denx.de> wrote: >> >> On 8/13/23 10:39, Loic Poulain wrote: >>> The SDP read register command can be used to read any memory >>> mapped address of the device (ddr, registers...). It can then >>> be exploited by an attacker to access sensitive data/values, >>> especially when running SDP from SPL, as SPL runs with highest >>> privileges in ARM secure mode. >>> >>> Without read, SDP still useful to bootstrap and jump on (signed) >>> blob such as u-boot with write and jump commands, but reading >>> is optional in that case (debug purpose). >>> >>> NXP SoCs usually have a dedicated SDP_READ_DISABLE fuse to disable >>> SDP read command in their ROM SDP implementation, so it seems quite >>> reasonable to make it optional from u-boot/spl as well. >> >> If there is a fuse, why not read the fuse and disable READ based on that >> fuse instead ? > > Well, fuse is more a way to tune a specific ROM code here, not the software. The way I read the commit message, when the fuse is set, the READ functionality should be disabled, to avoid any READs, right ? > It would be more generic to make it a build config like other features, and one > may purposely force SDP READ in SPL, even if disabled at ROM level. That > said we could also introduce a weak board_sdp_read_allowed() function... > > Let me know what you prefer. I think the weak default function would be a good approach. [...]
diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig index 1cfe602284..50cf7c0dae 100644 --- a/drivers/usb/gadget/Kconfig +++ b/drivers/usb/gadget/Kconfig @@ -195,6 +195,13 @@ config USB_FUNCTION_SDP allows to download images into memory and execute (jump to) them using the same protocol as implemented by the i.MX family's boot ROM. +config SDP_READ + bool "Enable SDP READ command" + depends on USB_FUNCTION_SDP + default n + help + Enable SDP secure sensitive SDP read register command. + config USB_FUNCTION_THOR bool "Enable USB THOR gadget" help @@ -344,6 +351,13 @@ config SPL_USB_SDP_SUPPORT allows to download images into memory and execute (jump to) them using the same protocol as implemented by the i.MX family's boot ROM. +config SPL_SDP_READ + bool "Enable SDP READ command in SPL" + depends on SPL_USB_SDP_SUPPORT + default n + help + Enable SDP secure sensitive SDP read register command. + config SPL_SDP_USB_DEV int "SDP USB controller index in SPL" default 0 diff --git a/drivers/usb/gadget/f_sdp.c b/drivers/usb/gadget/f_sdp.c index 4da5a160a0..48a68a50d9 100644 --- a/drivers/usb/gadget/f_sdp.c +++ b/drivers/usb/gadget/f_sdp.c @@ -310,6 +310,7 @@ static void sdp_rx_command_complete(struct usb_ep *ep, struct usb_request *req) be32_to_cpu(cmd->addr), be32_to_cpu(cmd->cnt)); switch (be16_to_cpu(cmd->cmd)) { +#if CONFIG_IS_ENABLED(SDP_READ) case SDP_READ_REGISTER: sdp->always_send_status = false; sdp->error_status = 0x0; @@ -321,6 +322,7 @@ static void sdp_rx_command_complete(struct usb_ep *ep, struct usb_request *req) printf("Reading %d registers at 0x%08x... ", sdp->dnl_bytes_remaining, sdp->dnl_address); break; +#endif case SDP_WRITE_FILE: sdp->always_send_status = true; sdp->error_status = SDP_WRITE_FILE_COMPLETE;
The SDP read register command can be used to read any memory mapped address of the device (ddr, registers...). It can then be exploited by an attacker to access sensitive data/values, especially when running SDP from SPL, as SPL runs with highest privileges in ARM secure mode. Without read, SDP still useful to bootstrap and jump on (signed) blob such as u-boot with write and jump commands, but reading is optional in that case (debug purpose). NXP SoCs usually have a dedicated SDP_READ_DISABLE fuse to disable SDP read command in their ROM SDP implementation, so it seems quite reasonable to make it optional from u-boot/spl as well. Signed-off-by: Loic Poulain <loic.poulain@linaro.org> --- drivers/usb/gadget/Kconfig | 14 ++++++++++++++ drivers/usb/gadget/f_sdp.c | 2 ++ 2 files changed, 16 insertions(+)