| Message ID | 20240210070934.2549994-5-swboyd@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | platform/chrome: Add DT USB/DP muxing/topology to Trogdor | expand |
Hi, On Fri, Feb 9, 2024 at 11:09 PM Stephen Boyd <swboyd@chromium.org> wrote: > > When a USB hub is described in DT, such as any device that matches the > onboard-hub driver, the connect_type is set to "unknown" or > USB_PORT_CONNECT_TYPE_UNKNOWN. This makes any device plugged into that > USB port report their 'removable' device attribute as "unknown". Improve > the connect_type attribute for ports, and in turn the removable > attribute for USB devices, by looking for child devices with a reg > property or an OF graph when the device is described in DT. > > If the graph exists, endpoints that are connected to a remote node must > be something like a usb-{a,b,c}-connector compatible node, or an > intermediate node like a redriver, and not a hardwired USB device on the > board. Set the connect_type to USB_PORT_CONNECT_TYPE_HOT_PLUG in this > case because the device is going to be plugged in. Set the connect_type > to USB_PORT_CONNECT_TYPE_HARD_WIRED if there's a child node for the port > like 'device@2' for port2. Set the connect_type to USB_PORT_NOT_USED if > there isn't an endpoint or child node corresponding to the port number. The above sounds good, but then I look at patch #18 ("dt-bindings: chrome: Add binding for ChromeOS Pogo pin connector") and patch #22 ("arm64: dts: qcom: sc7180-trogdor: Wire up USB and DP to usb-c-connectors") and it makes my Spidey Sense tingle. Specifically, I _think_ if a port is "hard wired" that can sometimes tell the system that the port is a bit more trusted. In the case of the "pogo" pins on detachables, though, I don't _think_ there's anything that prevents someone from making a "pogo to USB A port" adapter and then you could plug anything you wanted into the pogo port. If there's any extra trust given to these "internal" ports a nefarious attacker could presumably abuse that trust for the pogo pins. I have no idea if this is a realistic concern or not. I'm about 95% sure that hardwired "PCIe" ports get extra trust and get "deferred IOMMU flush" enabled and, in the case of PCIe, that actually is a real security hole. For USB, though, I think the system is more isolated by the USB host controller so I'm not sure that there is any extra trust given to "hard wired" ports. ...so maybe the answer here is to just ignore my rambling. ...or maybe the answer here is that everything is fine but patches #18 and #22 should be modified not to cause the pogo pins to be considered as "hard wired" since they really aren't... -Doug
Quoting Doug Anderson (2024-02-13 16:03:11) > Hi, > > On Fri, Feb 9, 2024 at 11:09 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > When a USB hub is described in DT, such as any device that matches the > > onboard-hub driver, the connect_type is set to "unknown" or > > USB_PORT_CONNECT_TYPE_UNKNOWN. This makes any device plugged into that > > USB port report their 'removable' device attribute as "unknown". Improve > > the connect_type attribute for ports, and in turn the removable > > attribute for USB devices, by looking for child devices with a reg > > property or an OF graph when the device is described in DT. > > > > If the graph exists, endpoints that are connected to a remote node must > > be something like a usb-{a,b,c}-connector compatible node, or an > > intermediate node like a redriver, and not a hardwired USB device on the > > board. Set the connect_type to USB_PORT_CONNECT_TYPE_HOT_PLUG in this > > case because the device is going to be plugged in. Set the connect_type > > to USB_PORT_CONNECT_TYPE_HARD_WIRED if there's a child node for the port > > like 'device@2' for port2. Set the connect_type to USB_PORT_NOT_USED if > > there isn't an endpoint or child node corresponding to the port number. > > The above sounds good, but then I look at patch #18 ("dt-bindings: > chrome: Add binding for ChromeOS Pogo pin connector") and patch #22 > ("arm64: dts: qcom: sc7180-trogdor: Wire up USB and DP to > usb-c-connectors") and it makes my Spidey Sense tingle. > > Specifically, I _think_ if a port is "hard wired" that can sometimes > tell the system that the port is a bit more trusted. In the case of > the "pogo" pins on detachables, though, I don't _think_ there's > anything that prevents someone from making a "pogo to USB A port" > adapter and then you could plug anything you wanted into the pogo > port. If there's any extra trust given to these "internal" ports a > nefarious attacker could presumably abuse that trust for the pogo > pins. The pogo pins on detachables are connected via the OF graph, so they are only ever going to be either "not connected" or "hotplug". They can't become "hard wired" because they're not a child node with a reg property corresponding to the USB port. > > I have no idea if this is a realistic concern or not. I'm about 95% > sure that hardwired "PCIe" ports get extra trust and get "deferred > IOMMU flush" enabled and, in the case of PCIe, that actually is a real > security hole. For USB, though, I think the system is more isolated by > the USB host controller so I'm not sure that there is any extra trust > given to "hard wired" ports. ...so maybe the answer here is to just > ignore my rambling. ...or maybe the answer here is that everything is > fine but patches #18 and #22 should be modified not to cause the pogo > pins to be considered as "hard wired" since they really aren't... > Pogo pins should be considered hot plug with this patch series, but I will double check that I didn't mess up that logic.
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c index c628c1abc907..622b8ada157c 100644 --- a/drivers/usb/core/port.c +++ b/drivers/usb/core/port.c @@ -9,6 +9,8 @@ #include <linux/kstrtox.h> #include <linux/slab.h> +#include <linux/of.h> +#include <linux/of_graph.h> #include <linux/pm_qos.h> #include <linux/component.h> @@ -696,7 +698,10 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1) { struct usb_port *port_dev; struct usb_device *hdev = hub->hdev; + struct device_node *np, *child, *ep, *remote_np, *port_np; int retval; + enum usb_port_connect_type connect_type = USB_PORT_CONNECT_TYPE_UNKNOWN; + u32 reg; port_dev = kzalloc(sizeof(*port_dev), GFP_KERNEL); if (!port_dev) @@ -708,6 +713,38 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1) return -ENOMEM; } + np = hdev->dev.of_node; + /* Only set connect_type if binding has ports/hardwired devices. */ + if (of_get_child_count(np)) + connect_type = USB_PORT_NOT_USED; + + /* Hotplug ports are connected and available in the OF graph. */ + if (of_graph_is_present(np)) { + port_np = of_graph_get_port_by_id(np, port1); + if (port_np) { + ep = of_graph_get_endpoint_by_regs(np, port1, -1); + if (ep) { + remote_np = of_graph_get_remote_port_parent(ep); + of_node_put(ep); + if (of_device_is_available(remote_np)) + connect_type = USB_PORT_CONNECT_TYPE_HOT_PLUG; + of_node_put(remote_np); + } + } + of_node_put(port_np); + } + + /* + * Hard-wired ports are child nodes with a reg property corresponding + * to the port number. + */ + for_each_available_child_of_node(np, child) { + if (!of_property_read_u32(child, "reg", ®) && reg == port1) + connect_type = USB_PORT_CONNECT_TYPE_HARD_WIRED; + } + + port_dev->connect_type = connect_type; + hub->ports[port1 - 1] = port_dev; port_dev->portnum = port1; set_bit(port1, hub->power_bits);
When a USB hub is described in DT, such as any device that matches the onboard-hub driver, the connect_type is set to "unknown" or USB_PORT_CONNECT_TYPE_UNKNOWN. This makes any device plugged into that USB port report their 'removable' device attribute as "unknown". Improve the connect_type attribute for ports, and in turn the removable attribute for USB devices, by looking for child devices with a reg property or an OF graph when the device is described in DT. If the graph exists, endpoints that are connected to a remote node must be something like a usb-{a,b,c}-connector compatible node, or an intermediate node like a redriver, and not a hardwired USB device on the board. Set the connect_type to USB_PORT_CONNECT_TYPE_HOT_PLUG in this case because the device is going to be plugged in. Set the connect_type to USB_PORT_CONNECT_TYPE_HARD_WIRED if there's a child node for the port like 'device@2' for port2. Set the connect_type to USB_PORT_NOT_USED if there isn't an endpoint or child node corresponding to the port number. To make sure things don't change, only set the port to not used if there are child nodes. This way an onboard hub connect_type doesn't change until ports are added or child nodes are added to describe hardwired devices. It's assumed that all ports or no ports will be described for a device. Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Matthias Kaehlcke <mka@chromium.org> Cc: <linux-usb@vger.kernel.org> Cc: <devicetree@vger.kernel.org> Cc: Pin-yen Lin <treapking@chromium.org> Cc: maciek swiech <drmasquatch@google.com> Signed-off-by: Stephen Boyd <swboyd@chromium.org> --- drivers/usb/core/port.c | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)