Re: [PATCH] USB: serial: io_ti: array underflow in edge_interrupt_callback()
On Tue, Aug 14, 2018 at 12:07:15PM +0300, Dan Carpenter wrote:
> A malicious USB device could set port_number to -3 and we would
> underflow the edge_serial->serial->port[] array.
Good catch!
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Dan Carpenter
>
> diff --git a/drivers/usb/serial/io_ti.c b/drivers/usb/serial/io_ti.c
> index 6d1d6efa3055..fa2af18c6efe 100644
> --- a/drivers/usb/serial/io_ti.c
> +++ b/drivers/usb/serial/io_ti.c
> @@ -1629,7 +1629,7 @@ static void edge_interrupt_callback(struct urb *urb)
> struct device *dev;
> unsigned char *data = urb->transfer_buffer;
> int length = urb->actual_length;
> - int port_number;
> + unsigned int port_number;
I'd prefer fixing the macro to never return a negative port number in
the first place instead of relying on conversion rules. These devices
only come with one or two ports and, while the protocol documentation is
hard to come by, it seems what we really want to do is just to check the
7th bit and thus change:
-#define TIUMP_GET_PORT_FROM_CODE(c)(((c) >> 4) - 3)
+#define TIUMP_GET_PORT_FROM_CODE(c)(((c) >> 6) & 0x01)
I only have a one-port device to test with, but I can at least confirm
that bits 0x30 are always set and that that's probably why subtraction
was used instead of masking out the relevant bit (i.e. it happened to
work).
This also avoids error messages such as
bad port number 4294967293
should the ignored bits (0xb0) have unexpected values (e.g. 0).
> int function;
> int retval;
> __u8 lsr;
Turns out we had the same issue in ti_usb_3410_5052 which is based on
io_ti, but where a recent change converted the port macro to a static
helper. In case you found this using your static checkers, you may want
to look into why it didn't catch that one.
I'll submit two fixes to address this as per above.
Thanks,
Johan
[PATCH 2/2] USB: serial: ti_usb_3410_5052: fix array underflow in completion handler
Similarly to a recently reported bug in io_ti, a malicious USB device
could set port_number to -3 and we would underflow the port array in the
interrupt completion handler.
As these devices only have one or two ports, fix this by making sure we
only consider the seventh bit when determining the port number (and
ignore bits 0xb0 which are typically set to 0x30).
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable
Signed-off-by: Johan Hovold
---
drivers/usb/serial/ti_usb_3410_5052.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/serial/ti_usb_3410_5052.c
b/drivers/usb/serial/ti_usb_3410_5052.c
index 3010878f7f8e..e3c5832337e0 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -1119,7 +1119,7 @@ static void ti_break(struct tty_struct *tty, int
break_state)
static int ti_get_port_from_code(unsigned char code)
{
- return (code >> 4) - 3;
+ return (code >> 6) & 0x01;
}
static int ti_get_func_from_code(unsigned char code)
--
2.18.0
[PATCH 1/2] USB: serial: io_ti: fix array underflow in completion handler
As reported by Dan Carpenter, a malicious USB device could set
port_number to -3 and we would underflow the port array in the interrupt
completion handler.
As these devices only have one or two ports, fix this by making sure we
only consider the seventh bit when determining the port number (and
ignore bits 0xb0 which are typically set to 0x30).
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable
Reported-by: Dan Carpenter
Signed-off-by: Johan Hovold
---
drivers/usb/serial/io_ti.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/serial/io_ti.h b/drivers/usb/serial/io_ti.h
index e53c68261017..9bbcee37524e 100644
--- a/drivers/usb/serial/io_ti.h
+++ b/drivers/usb/serial/io_ti.h
@@ -173,7 +173,7 @@ struct ump_interrupt {
} __attribute__((packed));
-#define TIUMP_GET_PORT_FROM_CODE(c)(((c) >> 4) - 3)
+#define TIUMP_GET_PORT_FROM_CODE(c)(((c) >> 6) & 0x01)
#define TIUMP_GET_FUNC_FROM_CODE(c)((c) & 0x0f)
#define TIUMP_INTERRUPT_CODE_LSR 0x03
#define TIUMP_INTERRUPT_CODE_MSR 0x04
--
2.18.0
Re: linux-next build broken for renesas-usb3?
On 20/08/2018 04:15, Yoshihiro Shimoda wrote: Hi John, Felipe, From: John Garry, Sent: Wednesday, August 15, 2018 9:55 PM On 10/08/2018 07:24, Felipe Balbi wrote: Hi Yoshihiro, I see Arnd is fixing it here: https://lore.kernel.org/patchwork/patch/974025/ I am not on that thread, so will comment here: > tristate 'Renesas USB3.0 Peripheral controller' > depends on ARCH_RENESAS || COMPILE_TEST > depends on EXTCON > +depends on USB || !USB Considering this following comment in the same Makefile, is this ok: NOTE: Gadget support ** DOES NOT ** depend on host-side CONFIG_USB !! Thank you very much for your reports and sorry for the delayed response. # I had a vacation in last week. I submitted a patch to fix the issue as below: https://patchwork.kernel.org/patch/10569847/ ok, thanks. It would have been better to cc me on that. John I think this patch can resolve the NOTE. Best regards, Yoshihiro Shimoda
[PATCH 3/6] usb: chipidea: Prevent unbalanced IRQ disable
The ChipIdea IRQ is disabled before scheduling the otg work and
re-enabled on otg work completion. However if the job is already
scheduled we have to undo the effect of disable_irq int order to
balance the IRQ disable-depth value.
Fixes: be6b0c1bd0be ("usb: chipidea: using one inline function to cover queue
work operations")
Signed-off-by: Loic Poulain
---
drivers/usb/chipidea/otg.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/chipidea/otg.h b/drivers/usb/chipidea/otg.h
index 7e7428e..4f8b817 100644
--- a/drivers/usb/chipidea/otg.h
+++ b/drivers/usb/chipidea/otg.h
@@ -17,7 +17,8 @@ void ci_handle_vbus_change(struct ci_hdrc *ci);
static inline void ci_otg_queue_work(struct ci_hdrc *ci)
{
disable_irq_nosync(ci->irq);
- queue_work(ci->wq, &ci->work);
+ if (queue_work(ci->wq, &ci->work) == false)
+ enable_irq(ci->irq);
}
#endif /* __DRIVERS_USB_CHIPIDEA_OTG_H */
--
2.7.4
[PATCH 4/6] usb: chipidea: Fix otg event handler
At OTG work running time, it's possible that several events need to be
addressed (e.g. ID and VBUS events). The current implementation handles
only one event at a time which leads to ignoring the other one. Fix it.
Signed-off-by: Loic Poulain
---
drivers/usb/chipidea/otg.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c
index db4ceff..f25d482 100644
--- a/drivers/usb/chipidea/otg.c
+++ b/drivers/usb/chipidea/otg.c
@@ -203,14 +203,17 @@ static void ci_otg_work(struct work_struct *work)
}
pm_runtime_get_sync(ci->dev);
+
if (ci->id_event) {
ci->id_event = false;
ci_handle_id_switch(ci);
- } else if (ci->b_sess_valid_event) {
+ }
+
+ if (ci->b_sess_valid_event) {
ci->b_sess_valid_event = false;
ci_handle_vbus_change(ci);
- } else
- dev_err(ci->dev, "unexpected event occurs at %s\n", __func__);
+ }
+
pm_runtime_put_sync(ci->dev);
enable_irq(ci->irq);
--
2.7.4
[PATCH 1/6] usb: chipidea: Add dynamic pinctrl selection
Some hardware implementations require to configure pins differently
according to the USB role (host/device), this can be an update of the
pins routing or a simple GPIO value change.
This patch introduces new optional "host" and "device" pinctrls.
If these pinctrls are defined by the device, they are respectively
selected on host/device role start.
If a default pinctrl exist, it is restored on host/device role stop.
Signed-off-by: Loic Poulain
---
drivers/usb/chipidea/core.c | 19 +++
drivers/usb/chipidea/host.c | 9 +
drivers/usb/chipidea/udc.c | 9 +
include/linux/usb/chipidea.h | 6 ++
4 files changed, 43 insertions(+)
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 85fc6db..03e52fc 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -46,6 +46,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -723,6 +724,24 @@ static int ci_get_platdata(struct device *dev,
else
cable->connected = false;
}
+
+ platdata->pctl = devm_pinctrl_get(dev);
+ if (!IS_ERR(platdata->pctl)) {
+ struct pinctrl_state *p;
+
+ p = pinctrl_lookup_state(platdata->pctl, "default");
+ if (!IS_ERR(p))
+ platdata->pins_default = p;
+
+ p = pinctrl_lookup_state(platdata->pctl, "host");
+ if (!IS_ERR(p))
+ platdata->pins_host = p;
+
+ p = pinctrl_lookup_state(platdata->pctl, "device");
+ if (!IS_ERR(p))
+ platdata->pins_device = p;
+ }
+
return 0;
}
diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
index af45aa32..55dbd49 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -13,6 +13,7 @@
#include
#include
#include
+#include
#include "../host/ehci.h"
@@ -150,6 +151,10 @@ static int host_start(struct ci_hdrc *ci)
}
}
+ if (ci->platdata->pins_host)
+ pinctrl_select_state(ci->platdata->pctl,
+ci->platdata->pins_host);
+
ret = usb_add_hcd(hcd, 0, 0);
if (ret) {
goto disable_reg;
@@ -194,6 +199,10 @@ static void host_stop(struct ci_hdrc *ci)
}
ci->hcd = NULL;
ci->otg.host = NULL;
+
+ if (ci->platdata->pins_host && ci->platdata->pins_default)
+ pinctrl_select_state(ci->platdata->pctl,
+ci->platdata->pins_default);
}
diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index 9852ec5..c04384e 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -19,6 +19,7 @@
#include
#include
#include
+#include
#include "ci.h"
#include "udc.h"
@@ -1965,6 +1966,10 @@ void ci_hdrc_gadget_destroy(struct ci_hdrc *ci)
static int udc_id_switch_for_device(struct ci_hdrc *ci)
{
+ if (ci->platdata->pins_device)
+ pinctrl_select_state(ci->platdata->pctl,
+ci->platdata->pins_device);
+
if (ci->is_otg)
/* Clear and enable BSV irq */
hw_write_otgsc(ci, OTGSC_BSVIS | OTGSC_BSVIE,
@@ -1983,6 +1988,10 @@ static void udc_id_switch_for_host(struct ci_hdrc *ci)
hw_write_otgsc(ci, OTGSC_BSVIE | OTGSC_BSVIS, OTGSC_BSVIS);
ci->vbus_active = 0;
+
+ if (ci->platdata->pins_device && ci->platdata->pins_default)
+ pinctrl_select_state(ci->platdata->pctl,
+ci->platdata->pins_default);
}
/**
diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
index 07f9936..63758c3 100644
--- a/include/linux/usb/chipidea.h
+++ b/include/linux/usb/chipidea.h
@@ -77,6 +77,12 @@ struct ci_hdrc_platform_data {
struct ci_hdrc_cablevbus_extcon;
struct ci_hdrc_cableid_extcon;
u32 phy_clkgate_delay_us;
+
+ /* pins */
+ struct pinctrl *pctl;
+ struct pinctrl_state *pins_default;
+ struct pinctrl_state *pins_host;
+ struct pinctrl_state *pins_device;
};
/* Default offset of capability registers */
--
2.7.4
[PATCH 5/6] phy: qcom-usb-hs: Fix unbalanced notifier registration
Phy power on/off cycle can happen several times during device life.
We then need to balance the extcon notifier registration accordingly.
Fixes: f0b5c2c96370 ("phy: qcom-usb-hs: Replace the extcon API")
Signed-off-by: Loic Poulain
---
drivers/phy/qualcomm/phy-qcom-usb-hs.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/phy/qualcomm/phy-qcom-usb-hs.c
b/drivers/phy/qualcomm/phy-qcom-usb-hs.c
index 2d0c70b..92e9d94 100644
--- a/drivers/phy/qualcomm/phy-qcom-usb-hs.c
+++ b/drivers/phy/qualcomm/phy-qcom-usb-hs.c
@@ -181,6 +181,12 @@ static int qcom_usb_hs_phy_power_off(struct phy *phy)
{
struct qcom_usb_hs_phy *uphy = phy_get_drvdata(phy);
+ if (uphy->vbus_edev) {
+ devm_extcon_unregister_notifier(&uphy->ulpi->dev,
+ uphy->vbus_edev, EXTCON_USB,
+ &uphy->vbus_notify);
+ }
+
regulator_disable(uphy->v3p3);
regulator_disable(uphy->v1p8);
clk_disable_unprepare(uphy->sleep_clk);
--
2.7.4
[PATCH 2/6] usb: chipidea: Support generic usb extcon
Add compatibility for extcon-usb-gpio which can handle more
than one cable per instance, allowing coherency of USB cable
states (USB/USB-HOST). These states can be generated from ID
or/and VBUS pins.
In case only one extcon device is associated to the USB device,
and this device supports USB and USB-HOST cable states, we now
use it for both VBUS (USB) and ID (USB-HOST) notifier.
Signed-off-by: Loic Poulain
---
drivers/usb/chipidea/core.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 03e52fc..c595718 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -699,6 +699,17 @@ static int ci_get_platdata(struct device *dev,
ext_id = extcon_get_edev_by_phandle(dev, 1);
if (IS_ERR(ext_id) && PTR_ERR(ext_id) != -ENODEV)
return PTR_ERR(ext_id);
+
+ /*
+* Some extcon devices like extcon-usb-gpio have only one
+* instance for both USB and USB-HOST cable states.
+*/
+ if (!IS_ERR(ext_vbus) && IS_ERR(ext_id)) {
+ if (extcon_get_state(ext_vbus, EXTCON_USB) >= 0 &&
+ extcon_get_state(ext_vbus, EXTCON_USB_HOST) >= 0) {
+ ext_id = ext_vbus;
+ }
+ }
}
cable = &platdata->vbus_extcon;
--
2.7.4
[PATCH 6/6] arm: dts: qcom: db410c: Enable USB OTG support
The Dragonboard-410c is able to act either as USB Host or Device.
The role can be determined at runtime via the USB_HS_ID pin which is
derived from the micro-usb port VBUS pin.
In Host role, SoC USB D+/D- are routed to the onboard USB 2.0 HUB.
In Device role, SoC USB D+/D- are routed to the USB 2.0 micro B port.
Routing is selected via USB_SW_SEL_PM gpio.
In device role USB HUB can be held in reset.
Signed-off-by: Loic Poulain
---
arch/arm64/boot/dts/qcom/apq8016-sbc-pmic-pins.dtsi | 20
arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi | 9 +
2 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/apq8016-sbc-pmic-pins.dtsi
b/arch/arm64/boot/dts/qcom/apq8016-sbc-pmic-pins.dtsi
index ec2f0de..99787cc 100644
--- a/arch/arm64/boot/dts/qcom/apq8016-sbc-pmic-pins.dtsi
+++ b/arch/arm64/boot/dts/qcom/apq8016-sbc-pmic-pins.dtsi
@@ -8,6 +8,16 @@
pinconf {
pins = "gpio3";
function = PMIC_GPIO_FUNC_NORMAL;
+ input-disable;
+ output-high;
+ };
+ };
+
+ usb_hub_reset_pm_device: usb_hub_reset_pm_device {
+ pinconf {
+ pins = "gpio3";
+ function = PMIC_GPIO_FUNC_NORMAL;
+ input-disable;
output-low;
};
};
@@ -22,6 +32,16 @@
};
};
+ usb_sw_sel_pm_device: usb_sw_sel_pm_device {
+ pinconf {
+ pins = "gpio4";
+ function = PMIC_GPIO_FUNC_NORMAL;
+ power-source = ;
+ input-disable;
+ output-low;
+ };
+ };
+
pm8916_gpios_leds: pm8916_gpios_leds {
pinconf {
pins = "gpio1", "gpio2";
diff --git a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
index 9ff8487..661a7fd 100644
--- a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
+++ b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
@@ -371,9 +371,10 @@
adp-disable;
hnp-disable;
srp-disable;
- dr_mode = "host";
- pinctrl-names = "default";
- pinctrl-0 = <&usb_sw_sel_pm>;
+ dr_mode = "otg";
+ pinctrl-names = "default", "device";
+ pinctrl-0 = <&usb_sw_sel_pm &usb_hub_reset_pm>;
+ pinctrl-1 = <&usb_sw_sel_pm_device
&usb_hub_reset_pm_device>;
ulpi {
phy {
v1p8-supply = <&pm8916_l7>;
@@ -512,7 +513,7 @@
usb_id: usb-id {
compatible = "linux,extcon-usb-gpio";
- vbus-gpio = <&msmgpio 121 GPIO_ACTIVE_HIGH>;
+ id-gpio = <&msmgpio 121 GPIO_ACTIVE_HIGH>;
pinctrl-names = "default";
pinctrl-0 = <&usb_id_default>;
};
--
2.7.4
Re: [PATCH 6/6] arm: dts: qcom: db410c: Enable USB OTG support
On Tue, Aug 21, 2018 at 8:56 AM Loic Poulain wrote:
>
> The Dragonboard-410c is able to act either as USB Host or Device.
> The role can be determined at runtime via the USB_HS_ID pin which is
> derived from the micro-usb port VBUS pin.
>
> In Host role, SoC USB D+/D- are routed to the onboard USB 2.0 HUB.
> In Device role, SoC USB D+/D- are routed to the USB 2.0 micro B port.
> Routing is selected via USB_SW_SEL_PM gpio.
>
> In device role USB HUB can be held in reset.
>
> Signed-off-by: Loic Poulain
> ---
> @@ -512,7 +513,7 @@
>
> usb_id: usb-id {
> compatible = "linux,extcon-usb-gpio";
> - vbus-gpio = <&msmgpio 121 GPIO_ACTIVE_HIGH>;
> + id-gpio = <&msmgpio 121 GPIO_ACTIVE_HIGH>;
The GPIO has magically changed from being connected to Vbus to ID? The
extcon binding is crap anyways...
Ideally, it would be nice if this was moved to the usb connector binding.
Rob
[PATCH] usb: musb: dsps: do not disable CPPI41 irq in driver teardown
TI AM335x CPPI41 module uses a single register bit for CPPI interrupts in
both musb controllers. So disabling the CPPI irq in one musb driver breaks
the other musb module.
Since musb is already disabled before tearing down dma controller in
musb_remove(), it is safe to not disable CPPI irq in
musb_dma_controller_destroy().
Fixes: 255348289f71 ("usb: musb: dsps: Manage CPPI 4.1 DMA interrupt in DSPS")
Cc: sta...@vger.kernel.org
Signed-off-by: Bin Liu
---
drivers/usb/musb/musb_dsps.c | 12 +---
1 file changed, 1 insertion(+), 11 deletions(-)
diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
index df827ff57b0d..23a0df79ef21 100644
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -658,16 +658,6 @@ dsps_dma_controller_create(struct musb *musb, void __iomem
*base)
return controller;
}
-static void dsps_dma_controller_destroy(struct dma_controller *c)
-{
- struct musb *musb = c->musb;
- struct dsps_glue *glue = dev_get_drvdata(musb->controller->parent);
- void __iomem *usbss_base = glue->usbss_base;
-
- musb_writel(usbss_base, USBSS_IRQ_CLEARR, USBSS_IRQ_PD_COMP);
- cppi41_dma_controller_destroy(c);
-}
-
#ifdef CONFIG_PM_SLEEP
static void dsps_dma_controller_suspend(struct dsps_glue *glue)
{
@@ -697,7 +687,7 @@ static struct musb_platform_ops dsps_ops = {
#ifdef CONFIG_USB_TI_CPPI41_DMA
.dma_init = dsps_dma_controller_create,
- .dma_exit = dsps_dma_controller_destroy,
+ .dma_exit = cppi41_dma_controller_destroy,
#endif
.enable = dsps_musb_enable,
.disable= dsps_musb_disable,
--
2.17.1
RE: linux-next build broken for renesas-usb3?
Hi, > From: John Garry, Sent: Tuesday, August 21, 2018 7:30 PM > > On 20/08/2018 04:15, Yoshihiro Shimoda wrote: > > Hi John, Felipe, > > > >> From: John Garry, Sent: Wednesday, August 15, 2018 9:55 PM > >> > >> On 10/08/2018 07:24, Felipe Balbi wrote: > >>> > >>> Hi Yoshihiro, > >>> > >> > >> I see Arnd is fixing it here: > >> https://lore.kernel.org/patchwork/patch/974025/ > >> > >> I am not on that thread, so will comment here: > >> >tristate 'Renesas USB3.0 Peripheral controller' > >> >depends on ARCH_RENESAS || COMPILE_TEST > >> >depends on EXTCON > >> > + depends on USB || !USB > >> > >> Considering this following comment in the same Makefile, is this ok: > >> NOTE: Gadget support ** DOES NOT ** depend on host-side CONFIG_USB !! > > > > Thank you very much for your reports and sorry for the delayed response. > > # I had a vacation in last week. > > > > I submitted a patch to fix the issue as below: > > https://patchwork.kernel.org/patch/10569847/ > > > > ok, thanks. It would have been better to cc me on that. Thank you for your confirmation! And I'm sorry for missing your email address to cc on that... Best regards, Yoshihiro Shimoda > John > > > I think this patch can resolve the NOTE. > > > > Best regards, > > Yoshihiro Shimoda > > > > > > >
