[PATCH v3 0/2] prevent incoherent can configuration in case of early return in the CAN netlink interface

[PATCH v3 0/2] prevent incoherent can configuration in case of early return in the CAN netlink interface

[Vincent Mailhol]

This series of two patch prevents, once for all, can_priv to be in an
inconsistent state in case of an early return in can_changelink() due
to invalid parameters.

* Changelog *

v2 -> v3:
  - Allocate the temporary struct can_priv on the heap instead of
    declaring it as static.
  - Split the patch into two to make it easier to backport to LTS
    kernels and add the "Fixes" tag.

v1 -> v2:
  - Change the prototype of can_calc_tdco() so that the changes are
    applied to the temporary priv instead of netdev_priv(dev).

Vincent Mailhol (2):
  can: netlink: prevent incoherent can configuration in case of early
    return
  can: bittiming: change can_calc_tdco()'s prototype to not directly
    modify priv

 drivers/net/can/dev/bittiming.c |  8 ++------
 drivers/net/can/dev/netlink.c   | 34 ++++++++++++++++++---------------
 include/linux/can/bittiming.h   |  7 +++++--
 3 files changed, 26 insertions(+), 23 deletions(-)

-- 
2.32.0

[PATCH v3 1/2] can: netlink: prevent incoherent can configuration in case of early return

[Vincent Mailhol]

struct can_priv has a set of flags (can_priv::ctrlmode) which are
correlated with the other fields of the structure. In
can_changelink(), those flags are set first and copied to can_priv. If
the function has to return early, for example due to an out of range
value provided by the user, then the global configuration might become
incoherent.

Example: the user provides an out of range dbitrate (e.g. 20
Mbps). The command fails (-EINVAL), however the FD flag was already
set resulting in a configuration where FD is on but the databittiming
parameters are empty.

* Illustration of above example *

| $ ip link set can0 type can bitrate 500000 dbitrate 20000000 fd on
| RTNETLINK answers: Invalid argument
| $ ip --details link show can0
| 1: can0: <NOARP,ECHO> mtu 72 qdisc noop state DOWN mode DEFAULT group default qlen 10
|     link/can  promiscuity 0 minmtu 0 maxmtu 0
|     can <FD> state STOPPED restart-ms 0
           ^^ FD flag is set without any of the databittiming parameters...
| 	  bitrate 500000 sample-point 0.875
| 	  tq 12 prop-seg 69 phase-seg1 70 phase-seg2 20 sjw 1
| 	  ES582.1/ES584.1: tseg1 2..256 tseg2 2..128 sjw 1..128 brp 1..512 brp-inc 1
| 	  ES582.1/ES584.1: dtseg1 2..32 dtseg2 1..16 dsjw 1..8 dbrp 1..32 dbrp-inc 1
| 	  clock 80000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535

To prevent this from happening, we do a local copy of can_priv, work
on it, an copy it at the very end of the function (i.e. only if all
previous checks succeeded).

Once this done, there is no more need to have a temporary variable for
a specific parameter. As such, the bittiming and data bittiming (bt
and dbt) are directly written to the temporary priv variable.


N.B. The temporary can_priv is too big to be allocated on the stack
because, on x86_64 sizeof(struct can_priv) is 448 and:

| $ objdump -d drivers/net/can/dev/netlink.o | ./scripts/checkstack.pl
| 0x00000000000002100 can_changelink []:            1200


Fixes: 9859ccd2c8be ("can: introduce the data bitrate configuration for CAN FD")
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/net/can/dev/netlink.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 80425636049d..21b76ca8cb22 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -58,14 +58,19 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 			  struct nlattr *data[],
 			  struct netlink_ext_ack *extack)
 {
-	struct can_priv *priv = netdev_priv(dev);
+	/* Work on a local copy of priv to prevent inconsistent value
+	 * in case of early return.
+	 */
+	static struct can_priv *priv;
 	int err;
 
 	/* We need synchronization with dev->stop() */
 	ASSERT_RTNL();
 
+	priv = kmemdup(netdev_priv(dev), sizeof(*priv), GFP_KERNEL);
+
 	if (data[IFLA_CAN_BITTIMING]) {
-		struct can_bittiming bt;
+		struct can_bittiming *bt = &priv->bittiming;
 
 		/* Do not allow changing bittiming while running */
 		if (dev->flags & IFF_UP)
@@ -79,22 +84,20 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 		if (!priv->bittiming_const && !priv->do_set_bittiming)
 			return -EOPNOTSUPP;
 
-		memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
-		err = can_get_bittiming(dev, &bt,
+		memcpy(bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(*bt));
+		err = can_get_bittiming(dev, bt,
 					priv->bittiming_const,
 					priv->bitrate_const,
 					priv->bitrate_const_cnt);
 		if (err)
 			return err;
 
-		if (priv->bitrate_max && bt.bitrate > priv->bitrate_max) {
+		if (priv->bitrate_max && bt->bitrate > priv->bitrate_max) {
 			netdev_err(dev, "arbitration bitrate surpasses transceiver capabilities of %d bps\n",
 				   priv->bitrate_max);
 			return -EINVAL;
 		}
 
-		memcpy(&priv->bittiming, &bt, sizeof(bt));
-
 		if (priv->do_set_bittiming) {
 			/* Finally, set the bit-timing registers */
 			err = priv->do_set_bittiming(dev);
@@ -158,7 +161,7 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 	}
 
 	if (data[IFLA_CAN_DATA_BITTIMING]) {
-		struct can_bittiming dbt;
+		struct can_bittiming *dbt = &priv->data_bittiming;
 
 		/* Do not allow changing bittiming while running */
 		if (dev->flags & IFF_UP)
@@ -172,23 +175,21 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 		if (!priv->data_bittiming_const && !priv->do_set_data_bittiming)
 			return -EOPNOTSUPP;
 
-		memcpy(&dbt, nla_data(data[IFLA_CAN_DATA_BITTIMING]),
-		       sizeof(dbt));
-		err = can_get_bittiming(dev, &dbt,
+		memcpy(dbt, nla_data(data[IFLA_CAN_DATA_BITTIMING]),
+		       sizeof(*dbt));
+		err = can_get_bittiming(dev, dbt,
 					priv->data_bittiming_const,
 					priv->data_bitrate_const,
 					priv->data_bitrate_const_cnt);
 		if (err)
 			return err;
 
-		if (priv->bitrate_max && dbt.bitrate > priv->bitrate_max) {
+		if (priv->bitrate_max && dbt->bitrate > priv->bitrate_max) {
 			netdev_err(dev, "canfd data bitrate surpasses transceiver capabilities of %d bps\n",
 				   priv->bitrate_max);
 			return -EINVAL;
 		}
 
-		memcpy(&priv->data_bittiming, &dbt, sizeof(dbt));
-
 		can_calc_tdco(dev);
 
 		if (priv->do_set_data_bittiming) {
@@ -223,6 +224,9 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 		priv->termination = termval;
 	}
 
+	memcpy(netdev_priv(dev), priv, sizeof(*priv));
+	kfree(priv);
+
 	return 0;
 }
 
-- 
2.32.0

[PATCH v3 2/2] can: bittiming: change can_calc_tdco()'s prototype to not directly modify priv

[Vincent Mailhol]

In previous commit, we introduced a temporary priv variable in
can_changelink(), wrote the changes to it and committed all changes at
the very end of the function.

However, the function can_calc_tdco() directly retrieves can_priv from
the net_device and directly modifies it. We change the prototype so
that it instead writes its changes to a struct can_priv that is passed
as an argument. This way, can_changelink() can pass the newly
introduced temporary priv variable.


Fixes: c25cc7993243 ("can: bittiming: add calculation for CAN FD Transmitter Delay Compensation (TDC)")
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/net/can/dev/bittiming.c | 8 ++------
 drivers/net/can/dev/netlink.c   | 2 +-
 include/linux/can/bittiming.h   | 7 +++++--
 3 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/net/can/dev/bittiming.c b/drivers/net/can/dev/bittiming.c
index f49170eadd54..bddd93e2e439 100644
--- a/drivers/net/can/dev/bittiming.c
+++ b/drivers/net/can/dev/bittiming.c
@@ -175,13 +175,9 @@ int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
 	return 0;
 }
 
-void can_calc_tdco(struct net_device *dev)
+void can_calc_tdco(struct can_tdc *tdc, const struct can_tdc_const *tdc_const,
+		   const struct can_bittiming *dbt)
 {
-	struct can_priv *priv = netdev_priv(dev);
-	const struct can_bittiming *dbt = &priv->data_bittiming;
-	struct can_tdc *tdc = &priv->tdc;
-	const struct can_tdc_const *tdc_const = priv->tdc_const;
-
 	if (!tdc_const)
 		return;
 
diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 21b76ca8cb22..66815ea6046e 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -190,7 +190,7 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 			return -EINVAL;
 		}
 
-		can_calc_tdco(dev);
+		can_calc_tdco(&priv->tdc, priv->tdc_const, &priv->data_bittiming);
 
 		if (priv->do_set_data_bittiming) {
 			/* Finally, set the bit-timing registers */
diff --git a/include/linux/can/bittiming.h b/include/linux/can/bittiming.h
index 9de6e9053e34..b3c1711ee0f0 100644
--- a/include/linux/can/bittiming.h
+++ b/include/linux/can/bittiming.h
@@ -87,7 +87,8 @@ struct can_tdc_const {
 int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
 		       const struct can_bittiming_const *btc);
 
-void can_calc_tdco(struct net_device *dev);
+void can_calc_tdco(struct can_tdc *tdc, const struct can_tdc_const *tdc_const,
+		   const struct can_bittiming *dbt);
 #else /* !CONFIG_CAN_CALC_BITTIMING */
 static inline int
 can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
@@ -97,7 +98,9 @@ can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
 	return -EINVAL;
 }
 
-static inline void can_calc_tdco(struct net_device *dev)
+static inline void
+can_calc_tdco(struct can_tdc *tdc, const struct can_tdc_const *tdc_const,
+	      const struct can_bittiming *dbt)
 {
 }
 #endif /* CONFIG_CAN_CALC_BITTIMING */
-- 
2.32.0

Re: [PATCH v3 1/2] can: netlink: prevent incoherent can configuration in case of early return

[Vincent MAILHOL]

On Tue. 7 Sep 2021 at 01:03, Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote:
> struct can_priv has a set of flags (can_priv::ctrlmode) which are
> correlated with the other fields of the structure. In
> can_changelink(), those flags are set first and copied to can_priv. If
> the function has to return early, for example due to an out of range
> value provided by the user, then the global configuration might become
> incoherent.
>
> Example: the user provides an out of range dbitrate (e.g. 20
> Mbps). The command fails (-EINVAL), however the FD flag was already
> set resulting in a configuration where FD is on but the databittiming
> parameters are empty.
>
> * Illustration of above example *
>
> | $ ip link set can0 type can bitrate 500000 dbitrate 20000000 fd on
> | RTNETLINK answers: Invalid argument
> | $ ip --details link show can0
> | 1: can0: <NOARP,ECHO> mtu 72 qdisc noop state DOWN mode DEFAULT group default qlen 10
> |     link/can  promiscuity 0 minmtu 0 maxmtu 0
> |     can <FD> state STOPPED restart-ms 0
>            ^^ FD flag is set without any of the databittiming parameters...
> |         bitrate 500000 sample-point 0.875
> |         tq 12 prop-seg 69 phase-seg1 70 phase-seg2 20 sjw 1
> |         ES582.1/ES584.1: tseg1 2..256 tseg2 2..128 sjw 1..128 brp 1..512 brp-inc 1
> |         ES582.1/ES584.1: dtseg1 2..32 dtseg2 1..16 dsjw 1..8 dbrp 1..32 dbrp-inc 1
> |         clock 80000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535
>
> To prevent this from happening, we do a local copy of can_priv, work
> on it, an copy it at the very end of the function (i.e. only if all
> previous checks succeeded).
>
> Once this done, there is no more need to have a temporary variable for
> a specific parameter. As such, the bittiming and data bittiming (bt
> and dbt) are directly written to the temporary priv variable.
>
>
> N.B. The temporary can_priv is too big to be allocated on the stack
> because, on x86_64 sizeof(struct can_priv) is 448 and:
>
> | $ objdump -d drivers/net/can/dev/netlink.o | ./scripts/checkstack.pl
> | 0x00000000000002100 can_changelink []:            1200
>
>
> Fixes: 9859ccd2c8be ("can: introduce the data bitrate configuration for CAN FD")
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
>  drivers/net/can/dev/netlink.c | 32 ++++++++++++++++++--------------
>  1 file changed, 18 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
> index 80425636049d..21b76ca8cb22 100644
> --- a/drivers/net/can/dev/netlink.c
> +++ b/drivers/net/can/dev/netlink.c
> @@ -58,14 +58,19 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
>                           struct nlattr *data[],
>                           struct netlink_ext_ack *extack)
>  {
> -       struct can_priv *priv = netdev_priv(dev);
> +       /* Work on a local copy of priv to prevent inconsistent value
> +        * in case of early return.
> +        */
> +       static struct can_priv *priv;
>         int err;
>
>         /* We need synchronization with dev->stop() */
>         ASSERT_RTNL();
>
> +       priv = kmemdup(netdev_priv(dev), sizeof(*priv), GFP_KERNEL);

Arg... I forgot to check the return value.
+       if (!priv)
+               return -ENOMEM;

I will send a v4, sorry for the noise.


Yours sincerely,
Vincent

Re: [PATCH v3 1/2] can: netlink: prevent incoherent can configuration in case of early return

[Vincent MAILHOL]

On Tue. 7 Sep. 2021 at 01:03, Vincent Mailhol
<mailhol.vincent@wanadoo.fr> wrote:
> struct can_priv has a set of flags (can_priv::ctrlmode) which are
> correlated with the other fields of the structure. In
> can_changelink(), those flags are set first and copied to can_priv. If
> the function has to return early, for example due to an out of range
> value provided by the user, then the global configuration might become
> incoherent.
>
> Example: the user provides an out of range dbitrate (e.g. 20
> Mbps). The command fails (-EINVAL), however the FD flag was already
> set resulting in a configuration where FD is on but the databittiming
> parameters are empty.
>
> * Illustration of above example *
>
> | $ ip link set can0 type can bitrate 500000 dbitrate 20000000 fd on
> | RTNETLINK answers: Invalid argument
> | $ ip --details link show can0
> | 1: can0: <NOARP,ECHO> mtu 72 qdisc noop state DOWN mode DEFAULT group default qlen 10
> |     link/can  promiscuity 0 minmtu 0 maxmtu 0
> |     can <FD> state STOPPED restart-ms 0
>            ^^ FD flag is set without any of the databittiming parameters...
> |         bitrate 500000 sample-point 0.875
> |         tq 12 prop-seg 69 phase-seg1 70 phase-seg2 20 sjw 1
> |         ES582.1/ES584.1: tseg1 2..256 tseg2 2..128 sjw 1..128 brp 1..512 brp-inc 1
> |         ES582.1/ES584.1: dtseg1 2..32 dtseg2 1..16 dsjw 1..8 dbrp 1..32 dbrp-inc 1
> |         clock 80000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535
>
> To prevent this from happening, we do a local copy of can_priv, work
> on it, an copy it at the very end of the function (i.e. only if all
> previous checks succeeded).
>
> Once this done, there is no more need to have a temporary variable for
> a specific parameter. As such, the bittiming and data bittiming (bt
> and dbt) are directly written to the temporary priv variable.
>
>
> N.B. The temporary can_priv is too big to be allocated on the stack
> because, on x86_64 sizeof(struct can_priv) is 448 and:
>
> | $ objdump -d drivers/net/can/dev/netlink.o | ./scripts/checkstack.pl
> | 0x00000000000002100 can_changelink []:            1200
>
>
> Fixes: 9859ccd2c8be ("can: introduce the data bitrate configuration for CAN FD")
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
>  drivers/net/can/dev/netlink.c | 32 ++++++++++++++++++--------------
>  1 file changed, 18 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
> index 80425636049d..21b76ca8cb22 100644
> --- a/drivers/net/can/dev/netlink.c
> +++ b/drivers/net/can/dev/netlink.c
> @@ -58,14 +58,19 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
>                           struct nlattr *data[],
>                           struct netlink_ext_ack *extack)
>  {
> -       struct can_priv *priv = netdev_priv(dev);
> +       /* Work on a local copy of priv to prevent inconsistent value
> +        * in case of early return.
> +        */
> +       static struct can_priv *priv;
>         int err;
>
>         /* We need synchronization with dev->stop() */
>         ASSERT_RTNL();
>
> +       priv = kmemdup(netdev_priv(dev), sizeof(*priv), GFP_KERNEL);
> +
>         if (data[IFLA_CAN_BITTIMING]) {
> -               struct can_bittiming bt;
> +               struct can_bittiming *bt = &priv->bittiming;
>
>                 /* Do not allow changing bittiming while running */
>                 if (dev->flags & IFF_UP)
> @@ -79,22 +84,20 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
>                 if (!priv->bittiming_const && !priv->do_set_bittiming)
>                         return -EOPNOTSUPP;
>
> -               memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
> -               err = can_get_bittiming(dev, &bt,
> +               memcpy(bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(*bt));
> +               err = can_get_bittiming(dev, bt,
>                                         priv->bittiming_const,
>                                         priv->bitrate_const,
>                                         priv->bitrate_const_cnt);
>                 if (err)
>                         return err;
>
> -               if (priv->bitrate_max && bt.bitrate > priv->bitrate_max) {
> +               if (priv->bitrate_max && bt->bitrate > priv->bitrate_max) {
>                         netdev_err(dev, "arbitration bitrate surpasses transceiver capabilities of %d bps\n",
>                                    priv->bitrate_max);
>                         return -EINVAL;
>                 }
>
> -               memcpy(&priv->bittiming, &bt, sizeof(bt));
> -
>                 if (priv->do_set_bittiming) {
>                         /* Finally, set the bit-timing registers */
>                         err = priv->do_set_bittiming(dev);

Actually, there is a big issue with my approach: the
do_set_bittiming() and some other callback functions need to
access priv. However, the changes being in the temporary
variable, these will not be visible by the device.

I will rework all that a little bit more before sending v4.


Yours sincerely,
Vincent Mailhol