can: fix handling of unmodifiable configuration options

commit bb208f144cf3f59d8f89a09a80efd04389718907 upstream. As described in 'can: m_can: tag current CAN FD controllers as non-ISO' (6cfda7fbebe) it is possible to define fixed configuration options by setting the according bit in 'ctrlmode' and clear it in 'ctrlmode_supported'. This leads to the incovenience that the fixed configuration bits can not be passed by netlink even when they have the correct values (e.g. non-ISO, FD). This patch fixes that issue and not only allows fixed set bit values to be set again but now requires(!) to provide these fixed values at configuration time. A valid CAN FD configuration consists of a nominal/arbitration bittiming, a data bittiming and a control mode with CAN_CTRLMODE_FD set - which is now enforced by a new can_validate() function. This fix additionally removed the inconsistency that was prohibiting the support of 'CANFD-only' controller drivers, like the RCar CAN FD. For this reason a new helper can_set_static_ctrlmode() has been introduced to provide a proper interface to handle static enabled CAN controller options. Reported-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net> Reviewed-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

can: fix handling of unmodifiable configuration options
commit bb208f144cf3f59d8f89a09a80efd04389718907 upstream. As described in 'can: m_can: tag current CAN FD controllers as non-ISO' (6cfda7fbebe) it is possible to define fixed configuration options by setting the according bit in 'ctrlmode' and clear it in 'ctrlmode_supported'. This leads to the incovenience that the fixed configuration bits can not be passed by netlink even when they have the correct values (e.g. non-ISO, FD). This patch fixes that issue and not only allows fixed set bit values to be set again but now requires(!) to provide these fixed values at configuration time. A valid CAN FD configuration consists of a nominal/arbitration bittiming, a data bittiming and a control mode with CAN_CTRLMODE_FD set - which is now enforced by a new can_validate() function. This fix additionally removed the inconsistency that was prohibiting the support of 'CANFD-only' controller drivers, like the RCar CAN FD. For this reason a new helper can_set_static_ctrlmode() has been introduced to provide a proper interface to handle static enabled CAN controller options. Reported-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net> Reviewed-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Oliver Hartkopp · Greg Kroah-Hartman
1 parent 9b68f12b57
Showing 3 changed files with 73 additions and 7 deletions Side-by-side Diff
drivers/net/can/dev.c
drivers/net/can/m_can/m_can.c
include/linux/can/dev.h
@@ -696,11 +696,17 @@
 	/* allow change of MTU according to the CANFD ability of the device */
 	switch (new_mtu) {
 	case CAN_MTU:
+		/* 'CANFD-only' controllers can not switch to CAN_MTU */
+		if (priv->ctrlmode_static & CAN_CTRLMODE_FD)
+			return -EINVAL;
+
 		priv->ctrlmode &= ~CAN_CTRLMODE_FD;
 		break;
  
 	case CANFD_MTU:
-		if (!(priv->ctrlmode_supported & CAN_CTRLMODE_FD))
+		/* check for potential CANFD ability */
+		if (!(priv->ctrlmode_supported & CAN_CTRLMODE_FD) &&
+		    !(priv->ctrlmode_static & CAN_CTRLMODE_FD))
 			return -EINVAL;
  
 		priv->ctrlmode |= CAN_CTRLMODE_FD;
@@ -782,6 +788,35 @@
 				= { .len = sizeof(struct can_bittiming_const) },
 };
  
+static int can_validate(struct nlattr *tb[], struct nlattr *data[])
+{
+	bool is_can_fd = false;
+
+	/* Make sure that valid CAN FD configurations always consist of
+	 * - nominal/arbitration bittiming
+	 * - data bittiming
+	 * - control mode with CAN_CTRLMODE_FD set
+	 */
+
+	if (data[IFLA_CAN_CTRLMODE]) {
+		struct can_ctrlmode *cm = nla_data(data[IFLA_CAN_CTRLMODE]);
+
+		is_can_fd = cm->flags & cm->mask & CAN_CTRLMODE_FD;
+	}
+
+	if (is_can_fd) {
+		if (!data[IFLA_CAN_BITTIMING] || !data[IFLA_CAN_DATA_BITTIMING])
+			return -EOPNOTSUPP;
+	}
+
+	if (data[IFLA_CAN_DATA_BITTIMING]) {
+		if (!is_can_fd || !data[IFLA_CAN_BITTIMING])
+			return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
 static int can_changelink(struct net_device *dev,
 			  struct nlattr *tb[], struct nlattr *data[])
 {
  
  
  
  
@@ -813,19 +848,31 @@
  
 	if (data[IFLA_CAN_CTRLMODE]) {
 		struct can_ctrlmode *cm;
+		u32 ctrlstatic;
+		u32 maskedflags;
  
 		/* Do not allow changing controller mode while running */
 		if (dev->flags & IFF_UP)
 			return -EBUSY;
 		cm = nla_data(data[IFLA_CAN_CTRLMODE]);
+		ctrlstatic = priv->ctrlmode_static;
+		maskedflags = cm->flags & cm->mask;
  
-		/* check whether changed bits are allowed to be modified */
-		if (cm->mask & ~priv->ctrlmode_supported)
+		/* check whether provided bits are allowed to be passed */
+		if (cm->mask & ~(priv->ctrlmode_supported | ctrlstatic))
 			return -EOPNOTSUPP;
  
+		/* do not check for static fd-non-iso if 'fd' is disabled */
+		if (!(maskedflags & CAN_CTRLMODE_FD))
+			ctrlstatic &= ~CAN_CTRLMODE_FD_NON_ISO;
+
+		/* make sure static options are provided by configuration */
+		if ((maskedflags & ctrlstatic) != ctrlstatic)
+			return -EOPNOTSUPP;
+
 		/* clear bits to be modified and copy the flag values */
 		priv->ctrlmode &= ~cm->mask;
-		priv->ctrlmode |= (cm->flags & cm->mask);
+		priv->ctrlmode |= maskedflags;
  
 		/* CAN_CTRLMODE_FD can only be set when driver supports FD */
 		if (priv->ctrlmode & CAN_CTRLMODE_FD)
@@ -966,6 +1013,7 @@
 	.maxtype	= IFLA_CAN_MAX,
 	.policy		= can_policy,
 	.setup		= can_setup,
+	.validate	= can_validate,
 	.newlink	= can_newlink,
 	.changelink	= can_changelink,
 	.get_size	= can_get_size,
@@ -955,7 +955,7 @@
 	priv->can.do_get_berr_counter = m_can_get_berr_counter;
  
 	/* CAN_CTRLMODE_FD_NON_ISO is fixed with M_CAN IP v3.0.1 */
-	priv->can.ctrlmode = CAN_CTRLMODE_FD_NON_ISO;
+	can_set_static_ctrlmode(dev, CAN_CTRLMODE_FD_NON_ISO);
  
 	/* CAN_CTRLMODE_FD_NON_ISO can not be changed with M_CAN IP v3.0.1 */
 	priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
@@ -40,9 +40,12 @@
 	struct can_clock clock;
  
 	enum can_state state;
-	u32 ctrlmode;
-	u32 ctrlmode_supported;
  
+	/* CAN controller features - see include/uapi/linux/can/netlink.h */
+	u32 ctrlmode;		/* current options setting */
+	u32 ctrlmode_supported;	/* options that can be modified by netlink */
+	u32 ctrlmode_static;	/* static enabled options for driver/hardware */
+
 	int restart_ms;
 	struct timer_list restart_timer;
  
@@ -106,6 +109,21 @@
 {
 	/* the CAN specific type of skb is identified by its data length */
 	return skb->len == CANFD_MTU;
+}
+
+/* helper to define static CAN controller features at device creation time */
+static inline void can_set_static_ctrlmode(struct net_device *dev,
+					   u32 static_mode)
+{
+	struct can_priv *priv = netdev_priv(dev);
+
+	/* alloc_candev() succeeded => netdev_priv() is valid at this point */
+	priv->ctrlmode = static_mode;
+	priv->ctrlmode_static = static_mode;
+
+	/* override MTU which was set by default in can_setup()? */
+	if (static_mode & CAN_CTRLMODE_FD)
+		dev->mtu = CANFD_MTU;
 }
  
 /* get data length from can_dlc with sanitized can_dlc */
...	...	@@ -696,11 +696,17 @@
696	696	/* allow change of MTU according to the CANFD ability of the device */
697	697	switch (new_mtu) {
698	698	case CAN_MTU:
	699	+ /* 'CANFD-only' controllers can not switch to CAN_MTU */
	700	+ if (priv->ctrlmode_static & CAN_CTRLMODE_FD)
	701	+ return -EINVAL;
	702	+
699	703	priv->ctrlmode &= ~CAN_CTRLMODE_FD;
700	704	break;
701	705
702	706	case CANFD_MTU:
703		- if (!(priv->ctrlmode_supported & CAN_CTRLMODE_FD))
	707	+ /* check for potential CANFD ability */
	708	+ if (!(priv->ctrlmode_supported & CAN_CTRLMODE_FD) &&
	709	+ !(priv->ctrlmode_static & CAN_CTRLMODE_FD))
704	710	return -EINVAL;
705	711
706	712	priv->ctrlmode \|= CAN_CTRLMODE_FD;
...	...	@@ -782,6 +788,35 @@
782	788	= { .len = sizeof(struct can_bittiming_const) },
783	789	};
784	790
	791	+static int can_validate(struct nlattr tb[], struct nlattr data[])
	792	+{
	793	+ bool is_can_fd = false;
	794	+
	795	+ /* Make sure that valid CAN FD configurations always consist of
	796	+ * - nominal/arbitration bittiming
	797	+ * - data bittiming
	798	+ * - control mode with CAN_CTRLMODE_FD set
	799	+ */
	800	+
	801	+ if (data[IFLA_CAN_CTRLMODE]) {
	802	+ struct can_ctrlmode *cm = nla_data(data[IFLA_CAN_CTRLMODE]);
	803	+
	804	+ is_can_fd = cm->flags & cm->mask & CAN_CTRLMODE_FD;
	805	+ }
	806	+
	807	+ if (is_can_fd) {
	808	+ if (!data[IFLA_CAN_BITTIMING] \|\| !data[IFLA_CAN_DATA_BITTIMING])
	809	+ return -EOPNOTSUPP;
	810	+ }
	811	+
	812	+ if (data[IFLA_CAN_DATA_BITTIMING]) {
	813	+ if (!is_can_fd \|\| !data[IFLA_CAN_BITTIMING])
	814	+ return -EOPNOTSUPP;
	815	+ }
	816	+
	817	+ return 0;
	818	+}
	819	+
785	820	static int can_changelink(struct net_device *dev,
786	821	struct nlattr tb[], struct nlattr data[])
787	822	{
788	823
789	824
790	825
791	826
...	...	@@ -813,19 +848,31 @@
813	848
814	849	if (data[IFLA_CAN_CTRLMODE]) {
815	850	struct can_ctrlmode *cm;
	851	+ u32 ctrlstatic;
	852	+ u32 maskedflags;
816	853
817	854	/* Do not allow changing controller mode while running */
818	855	if (dev->flags & IFF_UP)
819	856	return -EBUSY;
820	857	cm = nla_data(data[IFLA_CAN_CTRLMODE]);
	858	+ ctrlstatic = priv->ctrlmode_static;
	859	+ maskedflags = cm->flags & cm->mask;
821	860
822		- /* check whether changed bits are allowed to be modified */
823		- if (cm->mask & ~priv->ctrlmode_supported)
	861	+ /* check whether provided bits are allowed to be passed */
	862	+ if (cm->mask & ~(priv->ctrlmode_supported \| ctrlstatic))
824	863	return -EOPNOTSUPP;
825	864
	865	+ /* do not check for static fd-non-iso if 'fd' is disabled */
	866	+ if (!(maskedflags & CAN_CTRLMODE_FD))
	867	+ ctrlstatic &= ~CAN_CTRLMODE_FD_NON_ISO;
	868	+
	869	+ /* make sure static options are provided by configuration */
	870	+ if ((maskedflags & ctrlstatic) != ctrlstatic)
	871	+ return -EOPNOTSUPP;
	872	+
826	873	/* clear bits to be modified and copy the flag values */
827	874	priv->ctrlmode &= ~cm->mask;
828		- priv->ctrlmode \|= (cm->flags & cm->mask);
	875	+ priv->ctrlmode \|= maskedflags;
829	876
830	877	/* CAN_CTRLMODE_FD can only be set when driver supports FD */
831	878	if (priv->ctrlmode & CAN_CTRLMODE_FD)
...	...	@@ -966,6 +1013,7 @@
966	1013	.maxtype = IFLA_CAN_MAX,
967	1014	.policy = can_policy,
968	1015	.setup = can_setup,
	1016	+ .validate = can_validate,
969	1017	.newlink = can_newlink,
970	1018	.changelink = can_changelink,
971	1019	.get_size = can_get_size,
...	...	@@ -955,7 +955,7 @@
955	955	priv->can.do_get_berr_counter = m_can_get_berr_counter;
956	956
957	957	/* CAN_CTRLMODE_FD_NON_ISO is fixed with M_CAN IP v3.0.1 */
958		- priv->can.ctrlmode = CAN_CTRLMODE_FD_NON_ISO;
	958	+ can_set_static_ctrlmode(dev, CAN_CTRLMODE_FD_NON_ISO);
959	959
960	960	/* CAN_CTRLMODE_FD_NON_ISO can not be changed with M_CAN IP v3.0.1 */
961	961	priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK \|
...	...	@@ -40,9 +40,12 @@
40	40	struct can_clock clock;
41	41
42	42	enum can_state state;
43		- u32 ctrlmode;
44		- u32 ctrlmode_supported;
45	43
	44	+ /* CAN controller features - see include/uapi/linux/can/netlink.h */
	45	+ u32 ctrlmode; /* current options setting */
	46	+ u32 ctrlmode_supported; /* options that can be modified by netlink */
	47	+ u32 ctrlmode_static; /* static enabled options for driver/hardware */
	48	+
46	49	int restart_ms;
47	50	struct timer_list restart_timer;
48	51
...	...	@@ -106,6 +109,21 @@
106	109	{
107	110	/* the CAN specific type of skb is identified by its data length */
108	111	return skb->len == CANFD_MTU;
	112	+}
	113	+
	114	+/* helper to define static CAN controller features at device creation time */
	115	+static inline void can_set_static_ctrlmode(struct net_device *dev,
	116	+ u32 static_mode)
	117	+{
	118	+ struct can_priv *priv = netdev_priv(dev);
	119	+
	120	+ /* alloc_candev() succeeded => netdev_priv() is valid at this point */
	121	+ priv->ctrlmode = static_mode;
	122	+ priv->ctrlmode_static = static_mode;
	123	+
	124	+ /* override MTU which was set by default in can_setup()? */
	125	+ if (static_mode & CAN_CTRLMODE_FD)
	126	+ dev->mtu = CANFD_MTU;
109	127	}
110	128
111	129	/* get data length from can_dlc with sanitized can_dlc */