| Message ID | 1422601103-1144-1-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hi Shimoda-san, On Fri, Jan 30, 2015 at 7:58 AM, Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > This patch fixes an issue that the following commit causes NULL > pointer dereference in dma_release_channel(). > "usb: renesas_usbhs: add support for requesting DT DMA" > (commit id abd2dbf6bb1b5f3a03a8c76b1a8879da1dd30caa) Thanks for your patch! > The usbhsf_dma_init_dt() should set fifo->{t,r}x_chan to NULL if > dma_request_slave_channel_reason() returns IS_ERR value. > Otherwise, usbhsf_dma_quit() will call dma_release_channel(), and then > NULL pointer dereference happens. I guess it also causes other problems? The return value of usbhsf_dma_chan_get() is not checked for an error code in many callsites. > Reported-by: Simon Horman <horms@verge.net.au> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> Acked-by: Geert Uytterhoeven <geert@linux-m68k.org> > --- > This patch is based on Felipe's usb.git / testing/next branch. > (commit id =7db3990cb707ff91cd6507d53a0a730afe97) > > drivers/usb/renesas_usbhs/fifo.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/usb/renesas_usbhs/fifo.c b/drivers/usb/renesas_usbhs/fifo.c > index 4c086b1..d891bff 100644 > --- a/drivers/usb/renesas_usbhs/fifo.c > +++ b/drivers/usb/renesas_usbhs/fifo.c > @@ -1072,7 +1072,11 @@ static void usbhsf_dma_init_pdev(struct usbhs_fifo *fifo) > static void usbhsf_dma_init_dt(struct device *dev, struct usbhs_fifo *fifo) > { > fifo->tx_chan = dma_request_slave_channel_reason(dev, "tx"); > + if (IS_ERR(fifo->tx_chan)) > + fifo->tx_chan = NULL; > fifo->rx_chan = dma_request_slave_channel_reason(dev, "rx"); > + if (IS_ERR(fifo->rx_chan)) > + fifo->rx_chan = NULL; > } My first thought was: shouldn't it be handled in usbhsf_dma_init() instead, so it applies to both the legacy platform data and the DT case? But apparently dma_request_channel() returns NULL on failure, while dma_request_slave_channel_reason() returns an error code. Oh well... Remapping the error to NULL does mean the driver doesn't handle -EPROBE_DEFER, but that's tricky with DMA engines anyway, cfr. commit 55f5f9862a31e2ac ("i2c: sh_mobile: rework deferred probing") Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 30, 2015 at 03:58:23PM +0900, Yoshihiro Shimoda wrote: > This patch fixes an issue that the following commit causes NULL > pointer dereference in dma_release_channel(). > "usb: renesas_usbhs: add support for requesting DT DMA" > (commit id abd2dbf6bb1b5f3a03a8c76b1a8879da1dd30caa) > > The usbhsf_dma_init_dt() should set fifo->{t,r}x_chan to NULL if > dma_request_slave_channel_reason() returns IS_ERR value. > Otherwise, usbhsf_dma_quit() will call dma_release_channel(), and then > NULL pointer dereference happens. > > Reported-by: Simon Horman <horms@verge.net.au> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> I have tested this on the lager and koelsch boards using shmobile_defconfig and next-20150129 as a base. The environment where I originally observed the problem. Tested-by: Simon Horman <horms+renesas@verge.net.au> > --- > This patch is based on Felipe's usb.git / testing/next branch. > (commit id =7db3990cb707ff91cd6507d53a0a730afe97) > > drivers/usb/renesas_usbhs/fifo.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/usb/renesas_usbhs/fifo.c b/drivers/usb/renesas_usbhs/fifo.c > index 4c086b1..d891bff 100644 > --- a/drivers/usb/renesas_usbhs/fifo.c > +++ b/drivers/usb/renesas_usbhs/fifo.c > @@ -1072,7 +1072,11 @@ static void usbhsf_dma_init_pdev(struct usbhs_fifo *fifo) > static void usbhsf_dma_init_dt(struct device *dev, struct usbhs_fifo *fifo) > { > fifo->tx_chan = dma_request_slave_channel_reason(dev, "tx"); > + if (IS_ERR(fifo->tx_chan)) > + fifo->tx_chan = NULL; > fifo->rx_chan = dma_request_slave_channel_reason(dev, "rx"); > + if (IS_ERR(fifo->rx_chan)) > + fifo->rx_chan = NULL; > } > > static void usbhsf_dma_init(struct usbhs_priv *priv, > -- > 1.7.9.5 > -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert-san, > Hi Shimoda-san, > > On Fri, Jan 30, 2015 at 7:58 AM, Yoshihiro Shimoda > <yoshihiro.shimoda.uh@renesas.com> wrote: > > This patch fixes an issue that the following commit causes NULL > > pointer dereference in dma_release_channel(). > > "usb: renesas_usbhs: add support for requesting DT DMA" > > (commit id abd2dbf6bb1b5f3a03a8c76b1a8879da1dd30caa) > > Thanks for your patch! > > > The usbhsf_dma_init_dt() should set fifo->{t,r}x_chan to NULL if > > dma_request_slave_channel_reason() returns IS_ERR value. > > Otherwise, usbhsf_dma_quit() will call dma_release_channel(), and then > > NULL pointer dereference happens. > > I guess it also causes other problems? > The return value of usbhsf_dma_chan_get() is not checked for an error > code in many callsites. Yes, you are correct. So, I also should have tested this patch on PIO environment. > > Reported-by: Simon Horman <horms@verge.net.au> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > Acked-by: Geert Uytterhoeven <geert@linux-m68k.org> Thank you! This patch was already merged in Felipe's repository. Best regards, Yoshihiro Shimoda > > --- > > This patch is based on Felipe's usb.git / testing/next branch. > > (commit id =7db3990cb707ff91cd6507d53a0a730afe97) > > > > drivers/usb/renesas_usbhs/fifo.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/usb/renesas_usbhs/fifo.c b/drivers/usb/renesas_usbhs/fifo.c > > index 4c086b1..d891bff 100644 > > --- a/drivers/usb/renesas_usbhs/fifo.c > > +++ b/drivers/usb/renesas_usbhs/fifo.c > > @@ -1072,7 +1072,11 @@ static void usbhsf_dma_init_pdev(struct usbhs_fifo *fifo) > > static void usbhsf_dma_init_dt(struct device *dev, struct usbhs_fifo *fifo) > > { > > fifo->tx_chan = dma_request_slave_channel_reason(dev, "tx"); > > + if (IS_ERR(fifo->tx_chan)) > > + fifo->tx_chan = NULL; > > fifo->rx_chan = dma_request_slave_channel_reason(dev, "rx"); > > + if (IS_ERR(fifo->rx_chan)) > > + fifo->rx_chan = NULL; > > } > > My first thought was: shouldn't it be handled in usbhsf_dma_init() instead, > so it applies to both the legacy platform data and the DT case? > > But apparently dma_request_channel() returns NULL on failure, while > dma_request_slave_channel_reason() returns an error code. Oh well... > > Remapping the error to NULL does mean the driver doesn't handle > -EPROBE_DEFER, but that's tricky with DMA engines anyway, cfr. commit > 55f5f9862a31e2ac ("i2c: sh_mobile: rework deferred probing") > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
Hi Simon-san, > > On Fri, Jan 30, 2015 at 03:58:23PM +0900, Yoshihiro Shimoda wrote: > > This patch fixes an issue that the following commit causes NULL > > pointer dereference in dma_release_channel(). > > "usb: renesas_usbhs: add support for requesting DT DMA" > > (commit id abd2dbf6bb1b5f3a03a8c76b1a8879da1dd30caa) > > > > The usbhsf_dma_init_dt() should set fifo->{t,r}x_chan to NULL if > > dma_request_slave_channel_reason() returns IS_ERR value. > > Otherwise, usbhsf_dma_quit() will call dma_release_channel(), and then > > NULL pointer dereference happens. > > > > Reported-by: Simon Horman <horms@verge.net.au> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > I have tested this on the lager and koelsch boards using > shmobile_defconfig and next-20150129 as a base. The environment > where I originally observed the problem. > > Tested-by: Simon Horman <horms+renesas@verge.net.au> Thank you for the test! Best regards, Yoshihiro Shimoda > > --- > > This patch is based on Felipe's usb.git / testing/next branch. > > (commit id =7db3990cb707ff91cd6507d53a0a730afe97) > > > > drivers/usb/renesas_usbhs/fifo.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/usb/renesas_usbhs/fifo.c b/drivers/usb/renesas_usbhs/fifo.c > > index 4c086b1..d891bff 100644 > > --- a/drivers/usb/renesas_usbhs/fifo.c > > +++ b/drivers/usb/renesas_usbhs/fifo.c > > @@ -1072,7 +1072,11 @@ static void usbhsf_dma_init_pdev(struct usbhs_fifo *fifo) > > static void usbhsf_dma_init_dt(struct device *dev, struct usbhs_fifo *fifo) > > { > > fifo->tx_chan = dma_request_slave_channel_reason(dev, "tx"); > > + if (IS_ERR(fifo->tx_chan)) > > + fifo->tx_chan = NULL; > > fifo->rx_chan = dma_request_slave_channel_reason(dev, "rx"); > > + if (IS_ERR(fifo->rx_chan)) > > + fifo->rx_chan = NULL; > > } > > > > static void usbhsf_dma_init(struct usbhs_priv *priv, > > -- > > 1.7.9.5 > > -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/usb/renesas_usbhs/fifo.c b/drivers/usb/renesas_usbhs/fifo.c index 4c086b1..d891bff 100644 --- a/drivers/usb/renesas_usbhs/fifo.c +++ b/drivers/usb/renesas_usbhs/fifo.c @@ -1072,7 +1072,11 @@ static void usbhsf_dma_init_pdev(struct usbhs_fifo *fifo) static void usbhsf_dma_init_dt(struct device *dev, struct usbhs_fifo *fifo) { fifo->tx_chan = dma_request_slave_channel_reason(dev, "tx"); + if (IS_ERR(fifo->tx_chan)) + fifo->tx_chan = NULL; fifo->rx_chan = dma_request_slave_channel_reason(dev, "rx"); + if (IS_ERR(fifo->rx_chan)) + fifo->rx_chan = NULL; } static void usbhsf_dma_init(struct usbhs_priv *priv,
This patch fixes an issue that the following commit causes NULL pointer dereference in dma_release_channel(). "usb: renesas_usbhs: add support for requesting DT DMA" (commit id abd2dbf6bb1b5f3a03a8c76b1a8879da1dd30caa) The usbhsf_dma_init_dt() should set fifo->{t,r}x_chan to NULL if dma_request_slave_channel_reason() returns IS_ERR value. Otherwise, usbhsf_dma_quit() will call dma_release_channel(), and then NULL pointer dereference happens. Reported-by: Simon Horman <horms@verge.net.au> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- This patch is based on Felipe's usb.git / testing/next branch. (commit id =7db3990cb707ff91cd6507d53a0a730afe97) drivers/usb/renesas_usbhs/fifo.c | 4 ++++ 1 file changed, 4 insertions(+)