pull-request: can 2019-01-22

pull-request: can 2019-01-22

[Marc Kleine-Budde]

Hello David,

this is a pull request of 4 patches for net/master.

The first patch by is by Manfred Schlaegl and reverts a patch that caused wrong
warning messages in certain use cases. The next patch is by Oliver Hartkopp for
the bcm that adds sanity checks for the timer value before using it to detect
potential interger overflows. The last two patches are for the flexcan driver,
YueHaibing's patch fixes the the return value in the error path of the
flexcan_setup_stop_mode() function. The second patch is by Uwe Kleine-König and
fixes a NULL pointer deref on older flexcan cores in flexcan_chip_start().

regards,
Marc

---

The following changes since commit 49a57857aeea06ca831043acbb0fa5e0f50602fd:

  Linux 5.0-rc3 (2019-01-21 13:14:44 +1300)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can.git tags/linux-can-fixes-for-5.0-20190122

for you to fetch changes up to a55234dabe1f72cf22f9197980751d37e38ba020:

  can: flexcan: fix NULL pointer exception during bringup (2019-01-22 11:35:33 +0100)

----------------------------------------------------------------
linux-can-fixes-for-5.0-20190122

----------------------------------------------------------------
Manfred Schlaegl (1):
      can: dev: __can_get_echo_skb(): fix bogous check for non-existing skb by removing it

Oliver Hartkopp (1):
      can: bcm: check timer values before ktime conversion

Uwe Kleine-König (1):
      can: flexcan: fix NULL pointer exception during bringup

YueHaibing (1):
      can: flexcan: fix 'passing zero to ERR_PTR()' warning

 drivers/net/can/dev.c     | 27 +++++++++++++--------------
 drivers/net/can/flexcan.c |  4 ++--
 net/can/bcm.c             | 27 +++++++++++++++++++++++++++
 3 files changed, 42 insertions(+), 16 deletions(-)

regards,
Marc

[PATCH 1/4] can: dev: __can_get_echo_skb(): fix bogous check for non-existing skb by removing it

[Marc Kleine-Budde]

From: Manfred Schlaegl <manfred.schlaegl@ginzinger.com>

This patch revert commit 7da11ba5c506
("can: dev: __can_get_echo_skb(): print error message, if trying to echo non existing skb")

After introduction of this change we encountered following new error
message on various i.MX plattforms (flexcan):

| flexcan 53fc8000.can can0: __can_get_echo_skb: BUG! Trying to echo non
| existing skb: can_priv::echo_skb[0]

The introduction of the message was a mistake because
priv->echo_skb[idx] = NULL is a perfectly valid in following case: If
CAN_RAW_LOOPBACK is disabled (setsockopt) in applications, the pkt_type
of the tx skb's given to can_put_echo_skb is set to PACKET_LOOPBACK. In
this case can_put_echo_skb will not set priv->echo_skb[idx]. It is
therefore kept NULL.

As additional argument for revert: The order of check and usage of idx
was changed. idx is used to access an array element before checking it's
boundaries.

Signed-off-by: Manfred Schlaegl <manfred.schlaegl@ginzinger.com>
Fixes: 7da11ba5c506 ("can: dev: __can_get_echo_skb(): print error message, if trying to echo non existing skb")
Cc: linux-stable <stable@vger.kernel.org>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 3b3f88ffab53..c05e4d50d43d 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -480,8 +480,6 @@ EXPORT_SYMBOL_GPL(can_put_echo_skb);
 struct sk_buff *__can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr)
 {
 	struct can_priv *priv = netdev_priv(dev);
-	struct sk_buff *skb = priv->echo_skb[idx];
-	struct canfd_frame *cf;
 
 	if (idx >= priv->echo_skb_max) {
 		netdev_err(dev, "%s: BUG! Trying to access can_priv::echo_skb out of bounds (%u/max %u)\n",
@@ -489,20 +487,21 @@ struct sk_buff *__can_get_echo_skb(struct net_device *dev, unsigned int idx, u8
 		return NULL;
 	}
 
-	if (!skb) {
-		netdev_err(dev, "%s: BUG! Trying to echo non existing skb: can_priv::echo_skb[%u]\n",
-			   __func__, idx);
-		return NULL;
-	}
+	if (priv->echo_skb[idx]) {
+		/* Using "struct canfd_frame::len" for the frame
+		 * length is supported on both CAN and CANFD frames.
+		 */
+		struct sk_buff *skb = priv->echo_skb[idx];
+		struct canfd_frame *cf = (struct canfd_frame *)skb->data;
+		u8 len = cf->len;
 
-	/* Using "struct canfd_frame::len" for the frame
-	 * length is supported on both CAN and CANFD frames.
-	 */
-	cf = (struct canfd_frame *)skb->data;
-	*len_ptr = cf->len;
-	priv->echo_skb[idx] = NULL;
+		*len_ptr = len;
+		priv->echo_skb[idx] = NULL;
 
-	return skb;
+		return skb;
+	}
+
+	return NULL;
 }
 
 /*
-- 
2.20.1

[PATCH 2/4] can: bcm: check timer values before ktime conversion

[Marc Kleine-Budde]

From: Oliver Hartkopp <socketcan@hartkopp.net>

Kyungtae Kim detected a potential integer overflow in bcm_[rx|tx]_setup()
when the conversion into ktime multiplies the given value with NSEC_PER_USEC
(1000).

Reference: https://marc.info/?l=linux-can&m=154732118819828&w=2

Add a check for the given tv_usec, so that the value stays below one second.
Additionally limit the tv_sec value to a reasonable value for CAN related
use-cases of 400 days and ensure all values to be positive.

Reported-by: Kyungtae Kim <kt0755@gmail.com>
Tested-by: Oliver Hartkopp <socketcan@hartkopp.net>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
Cc: linux-stable <stable@vger.kernel.org> # >= 2.6.26
Tested-by: Kyungtae Kim <kt0755@gmail.com>
Acked-by: Andre Naujoks <nautsch2@gmail.com>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 net/can/bcm.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/net/can/bcm.c b/net/can/bcm.c
index 0af8f0db892a..79bb8afa9c0c 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -67,6 +67,9 @@
  */
 #define MAX_NFRAMES 256
 
+/* limit timers to 400 days for sending/timeouts */
+#define BCM_TIMER_SEC_MAX (400 * 24 * 60 * 60)
+
 /* use of last_frames[index].flags */
 #define RX_RECV    0x40 /* received data for this element */
 #define RX_THR     0x80 /* element not been sent due to throttle feature */
@@ -140,6 +143,22 @@ static inline ktime_t bcm_timeval_to_ktime(struct bcm_timeval tv)
 	return ktime_set(tv.tv_sec, tv.tv_usec * NSEC_PER_USEC);
 }
 
+/* check limitations for timeval provided by user */
+static bool bcm_is_invalid_tv(struct bcm_msg_head *msg_head)
+{
+	if ((msg_head->ival1.tv_sec < 0) ||
+	    (msg_head->ival1.tv_sec > BCM_TIMER_SEC_MAX) ||
+	    (msg_head->ival1.tv_usec < 0) ||
+	    (msg_head->ival1.tv_usec >= USEC_PER_SEC) ||
+	    (msg_head->ival2.tv_sec < 0) ||
+	    (msg_head->ival2.tv_sec > BCM_TIMER_SEC_MAX) ||
+	    (msg_head->ival2.tv_usec < 0) ||
+	    (msg_head->ival2.tv_usec >= USEC_PER_SEC))
+		return true;
+
+	return false;
+}
+
 #define CFSIZ(flags) ((flags & CAN_FD_FRAME) ? CANFD_MTU : CAN_MTU)
 #define OPSIZ sizeof(struct bcm_op)
 #define MHSIZ sizeof(struct bcm_msg_head)
@@ -873,6 +892,10 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 	if (msg_head->nframes < 1 || msg_head->nframes > MAX_NFRAMES)
 		return -EINVAL;
 
+	/* check timeval limitations */
+	if ((msg_head->flags & SETTIMER) && bcm_is_invalid_tv(msg_head))
+		return -EINVAL;
+
 	/* check the given can_id */
 	op = bcm_find_op(&bo->tx_ops, msg_head, ifindex);
 	if (op) {
@@ -1053,6 +1076,10 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 	     (!(msg_head->can_id & CAN_RTR_FLAG))))
 		return -EINVAL;
 
+	/* check timeval limitations */
+	if ((msg_head->flags & SETTIMER) && bcm_is_invalid_tv(msg_head))
+		return -EINVAL;
+
 	/* check the given can_id */
 	op = bcm_find_op(&bo->rx_ops, msg_head, ifindex);
 	if (op) {
-- 
2.20.1

[PATCH 3/4] can: flexcan: fix 'passing zero to ERR_PTR()' warning

[Marc Kleine-Budde]

From: YueHaibing <yuehaibing@huawei.com>

Fix a static code checker warning:
drivers/net/can/flexcan.c:1435 flexcan_setup_stop_mode() warn: passing zero to 'PTR_ERR'

Fixes: de3578c198c6 ("can: flexcan: add self wakeup support")
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/flexcan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 0f36eafe3ac1..5f097648d12d 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -1432,7 +1432,7 @@ static int flexcan_setup_stop_mode(struct platform_device *pdev)
 	gpr_np = of_find_node_by_phandle(phandle);
 	if (!gpr_np) {
 		dev_dbg(&pdev->dev, "could not find gpr node by phandle\n");
-		return PTR_ERR(gpr_np);
+		return -ENODEV;
 	}
 
 	priv = netdev_priv(dev);
-- 
2.20.1

[PATCH 4/4] can: flexcan: fix NULL pointer exception during bringup

[Marc Kleine-Budde]

From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Commit cbffaf7aa09e ("can: flexcan: Always use last mailbox for TX")
introduced a loop letting i run up to (including) ARRAY_SIZE(regs->mb)
and in the body accessed regs->mb[i] which is an out-of-bounds array
access that then resulted in an access to an reserved register area.

Later this was changed by commit 0517961ccdf1 ("can: flexcan: Add
provision for variable payload size") to iterate a bit differently but
still runs one iteration too much resulting to call

	flexcan_get_mb(priv, priv->mb_count)

which results in a WARN_ON and then a NULL pointer exception. This
only affects devices compatible with "fsl,p1010-flexcan",
"fsl,imx53-flexcan", "fsl,imx35-flexcan", "fsl,imx25-flexcan",
"fsl,imx28-flexcan", so newer i.MX SoCs are not affected.

Fixes: cbffaf7aa09e ("can: flexcan: Always use last mailbox for TX")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Cc: linux-stable <stable@vger.kernel.org> # >= 4.20
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/flexcan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 5f097648d12d..1c66fb2ad76b 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -1106,7 +1106,7 @@ static int flexcan_chip_start(struct net_device *dev)
 		}
 	} else {
 		/* clear and invalidate unused mailboxes first */
-		for (i = FLEXCAN_TX_MB_RESERVED_OFF_FIFO; i <= priv->mb_count; i++) {
+		for (i = FLEXCAN_TX_MB_RESERVED_OFF_FIFO; i < priv->mb_count; i++) {
 			mb = flexcan_get_mb(priv, i);
 			priv->write(FLEXCAN_MB_CODE_RX_INACTIVE,
 				    &mb->can_ctrl);
-- 
2.20.1

Re: [PATCH 4/4] can: flexcan: fix NULL pointer exception during bringup

[Uwe Kleine-König]

On Tue, Jan 22, 2019 at 02:21:16PM +0100, Marc Kleine-Budde wrote:
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Commit cbffaf7aa09e ("can: flexcan: Always use last mailbox for TX")
> introduced a loop letting i run up to (including) ARRAY_SIZE(regs->mb)
> and in the body accessed regs->mb[i] which is an out-of-bounds array
> access that then resulted in an access to an reserved register area.
> 
> Later this was changed by commit 0517961ccdf1 ("can: flexcan: Add
> provision for variable payload size") to iterate a bit differently but
> still runs one iteration too much resulting to call
> 
> 	flexcan_get_mb(priv, priv->mb_count)
> 
> which results in a WARN_ON and then a NULL pointer exception. This
> only affects devices compatible with "fsl,p1010-flexcan",
> "fsl,imx53-flexcan", "fsl,imx35-flexcan", "fsl,imx25-flexcan",
> "fsl,imx28-flexcan", so newer i.MX SoCs are not affected.
> 
> Fixes: cbffaf7aa09e ("can: flexcan: Always use last mailbox for TX")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Cc: linux-stable <stable@vger.kernel.org> # >= 4.20

Given that cbffaf7aa09e was backported to v4.19.x a fix is needed there,
too. The patch looks different but I already sent a patch to
stable@vger.k.o.

Not sure if ">= 4.20" should be adapted accordingly. cbffaf7aa09e was
backported resulting in commit 24e5589791d0 which made it into v4.19.6.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 4/4] can: flexcan: fix NULL pointer exception during bringup

[Uwe Kleine-König]

Hello Sasha,

On Wed, Jan 23, 2019 at 10:57:44PM +0000, Sasha Levin wrote:
> [This is an automated email]

Not sure if I only state the obvious that was just missed by your
automation.
> 
> This commit has been processed because it contains a "Fixes:" tag,
> fixing commit: cbffaf7aa09e can: flexcan: Always use last mailbox for TX.
> 
> The bot has tested the following trees: v4.20.3, v4.19.16.
> 
> v4.20.3: Failed to apply! Possible dependencies:
>     0517961ccdf1 ("can: flexcan: Add provision for variable payload size")
>     22233f7bf2c9 ("can: flexcan: FLEXCAN_IFLAG_MB: add () around macro argument")
>     5156c7b11f35 ("can: flexcan: move rx_offload_add() from flexcan_probe() to flexcan_open()")
>     7ad0f53a394b ("can: flexcan: flexcan_chip_start(): enable loopback mode in flexcan")
>     de3578c198c6 ("can: flexcan: add self wakeup support")
> 
> v4.19.16: Failed to apply! Possible dependencies:
>     0517961ccdf1 ("can: flexcan: Add provision for variable payload size")
>     22233f7bf2c9 ("can: flexcan: FLEXCAN_IFLAG_MB: add () around macro argument")
>     5156c7b11f35 ("can: flexcan: move rx_offload_add() from flexcan_probe() to flexcan_open()")
>     7ad0f53a394b ("can: flexcan: flexcan_chip_start(): enable loopback mode in flexcan")
>     de3578c198c6 ("can: flexcan: add self wakeup support")
> 
> 
> How should we proceed with this patch?

When I posted the commit I also posted one that fits on top of v4.19.x.
I would expect that the same also fits on v4.20.x. If not, tell me.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/4] can: bcm: check timer values before ktime conversion

[Oliver Hartkopp]

Hi Shasha,

On 23.01.19 23:57, Sasha Levin wrote:
> Hi,
> 
> [This is an automated email]
> 
> This commit has been processed because it contains a -stable tag.
> The stable tag indicates that it's relevant for the following trees: 2.6.26+
> 
> The bot has tested the following trees: v4.20.3, v4.19.16, v4.14.94, v4.9.151, v4.4.171, v3.18.132.
> 
> v4.20.3: Build OK!
> v4.19.16: Build OK!
> v4.14.94: Build OK!
> v4.9.151: Build OK!
> v4.4.171: Failed to apply! Possible dependencies:
>      2b5f5f5dc114 ("can: bcm: unify bcm_msg_head handling and prepare function parameters")
>      6f3b911d5f29 ("can: bcm: add support for CAN FD frames")
>      72c8a89ad2e4 ("can: bcm: use CAN frame instead of can_frame in comments")
>      95acb490ec51 ("can: bcm: fix indention and other minor style issues")
> 
> v3.18.132: Failed to apply! Possible dependencies:
>      069f8457ae52 ("can: fix spelling errors")
>      2b5f5f5dc114 ("can: bcm: unify bcm_msg_head handling and prepare function parameters")
>      6ce8e9ce5989 ("new helper: memcpy_from_msg()")
>      6f3b911d5f29 ("can: bcm: add support for CAN FD frames")
>      72c8a89ad2e4 ("can: bcm: use CAN frame instead of can_frame in comments")
>      95acb490ec51 ("can: bcm: fix indention and other minor style issues")
>      ba61a8d9d780 ("can: avoid using timeval for uapi")
> 
> 
> How should we proceed with this patch?

As we do have a proper upstream commit hash now ...

https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can.git/commit/?h=testing&id=93171ba6f1deffd82f381d36cb13177872d023f6

... here is the ported patch for kernels pre version 4.8:

https://marc.info/?l=linux-can&m=154832094402622&w=2

Thanks,
Oliver

ps. the pull request to Dave is still hanging here:

https://marc.info/?l=linux-can&m=154831478900596&w=2

Re: pull-request: can 2019-01-22

[David Miller]

From: Marc Kleine-Budde <mkl@pengutronix.de>
Date: Tue, 22 Jan 2019 14:21:12 +0100

> this is a pull request of 4 patches for net/master.
> 
> The first patch by is by Manfred Schlaegl and reverts a patch that caused wrong
> warning messages in certain use cases. The next patch is by Oliver Hartkopp for
> the bcm that adds sanity checks for the timer value before using it to detect
> potential interger overflows. The last two patches are for the flexcan driver,
> YueHaibing's patch fixes the the return value in the error path of the
> flexcan_setup_stop_mode() function. The second patch is by Uwe Kleine-König and
> fixes a NULL pointer deref on older flexcan cores in flexcan_chip_start().

Pulled, thanks Marc.

Re: [PATCH 4/4] can: flexcan: fix NULL pointer exception during bringup

[Greg KH]

On Thu, Jan 24, 2019 at 08:26:10AM +0100, Uwe Kleine-König wrote:
> Hello Sasha,
> 
> On Wed, Jan 23, 2019 at 10:57:44PM +0000, Sasha Levin wrote:
> > [This is an automated email]
> 
> Not sure if I only state the obvious that was just missed by your
> automation.
> > 
> > This commit has been processed because it contains a "Fixes:" tag,
> > fixing commit: cbffaf7aa09e can: flexcan: Always use last mailbox for TX.
> > 
> > The bot has tested the following trees: v4.20.3, v4.19.16.
> > 
> > v4.20.3: Failed to apply! Possible dependencies:
> >     0517961ccdf1 ("can: flexcan: Add provision for variable payload size")
> >     22233f7bf2c9 ("can: flexcan: FLEXCAN_IFLAG_MB: add () around macro argument")
> >     5156c7b11f35 ("can: flexcan: move rx_offload_add() from flexcan_probe() to flexcan_open()")
> >     7ad0f53a394b ("can: flexcan: flexcan_chip_start(): enable loopback mode in flexcan")
> >     de3578c198c6 ("can: flexcan: add self wakeup support")
> > 
> > v4.19.16: Failed to apply! Possible dependencies:
> >     0517961ccdf1 ("can: flexcan: Add provision for variable payload size")
> >     22233f7bf2c9 ("can: flexcan: FLEXCAN_IFLAG_MB: add () around macro argument")
> >     5156c7b11f35 ("can: flexcan: move rx_offload_add() from flexcan_probe() to flexcan_open()")
> >     7ad0f53a394b ("can: flexcan: flexcan_chip_start(): enable loopback mode in flexcan")
> >     de3578c198c6 ("can: flexcan: add self wakeup support")
> > 
> > 
> > How should we proceed with this patch?
> 
> When I posted the commit I also posted one that fits on top of v4.19.x.
> I would expect that the same also fits on v4.20.x. If not, tell me.

Ah, I see that now, sorry for the noise of the FAILED email...