* [PATCH] can: netlink: prevent incoherent can configuration in case of early return
@ 2021-08-20 9:03 Vincent Mailhol
2021-08-23 2:44 ` Vincent MAILHOL
0 siblings, 1 reply; 2+ messages in thread
From: Vincent Mailhol @ 2021-08-20 9:03 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can; +Cc: netdev, linux-kernel, Vincent Mailhol
struct can_priv has a set of flags (can_priv::ctrlmode) which are
correlated with the other field 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.
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
Hi Marc,
Do you think this need a "Fixes" tag?
If yes, feel free to add this line to the patch:
Fixes: 9859ccd2c8be ("can: introduce the data bitrate configuration for CAN FD")
---
drivers/net/can/dev/netlink.c | 86 ++++++++++++++++++-----------------
1 file changed, 45 insertions(+), 41 deletions(-)
diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 80425636049d..6a14c51a058b 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -58,14 +58,20 @@ 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. net/core/rtnetlink.c has a global
+ * mutex so static declaration is OK
+ */
+ static struct can_priv priv;
int err;
/* We need synchronization with dev->stop() */
ASSERT_RTNL();
+ memcpy(&priv, netdev_priv(dev), sizeof(priv));
+
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)
@@ -76,28 +82,26 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
* directly via do_set_bitrate(). Bail out if neither
* is given.
*/
- if (!priv->bittiming_const && !priv->do_set_bittiming)
+ 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,
- priv->bittiming_const,
- priv->bitrate_const,
- priv->bitrate_const_cnt);
+ 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);
+ priv.bitrate_max);
return -EINVAL;
}
- memcpy(&priv->bittiming, &bt, sizeof(bt));
-
- if (priv->do_set_bittiming) {
+ if (priv.do_set_bittiming) {
/* Finally, set the bit-timing registers */
- err = priv->do_set_bittiming(dev);
+ err = priv.do_set_bittiming(dev);
if (err)
return err;
}
@@ -112,11 +116,11 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
if (dev->flags & IFF_UP)
return -EBUSY;
cm = nla_data(data[IFLA_CAN_CTRLMODE]);
- ctrlstatic = priv->ctrlmode_static;
+ ctrlstatic = priv.ctrlmode_static;
maskedflags = cm->flags & cm->mask;
/* check whether provided bits are allowed to be passed */
- if (maskedflags & ~(priv->ctrlmode_supported | ctrlstatic))
+ if (maskedflags & ~(priv.ctrlmode_supported | ctrlstatic))
return -EOPNOTSUPP;
/* do not check for static fd-non-iso if 'fd' is disabled */
@@ -128,16 +132,16 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
return -EOPNOTSUPP;
/* clear bits to be modified and copy the flag values */
- priv->ctrlmode &= ~cm->mask;
- priv->ctrlmode |= maskedflags;
+ priv.ctrlmode &= ~cm->mask;
+ priv.ctrlmode |= maskedflags;
/* CAN_CTRLMODE_FD can only be set when driver supports FD */
- if (priv->ctrlmode & CAN_CTRLMODE_FD) {
+ if (priv.ctrlmode & CAN_CTRLMODE_FD) {
dev->mtu = CANFD_MTU;
} else {
dev->mtu = CAN_MTU;
- memset(&priv->data_bittiming, 0,
- sizeof(priv->data_bittiming));
+ memset(&priv.data_bittiming, 0,
+ sizeof(priv.data_bittiming));
}
}
@@ -145,7 +149,7 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
/* Do not allow changing restart delay while running */
if (dev->flags & IFF_UP)
return -EBUSY;
- priv->restart_ms = nla_get_u32(data[IFLA_CAN_RESTART_MS]);
+ priv.restart_ms = nla_get_u32(data[IFLA_CAN_RESTART_MS]);
}
if (data[IFLA_CAN_RESTART]) {
@@ -158,7 +162,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)
@@ -169,31 +173,29 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
* directly via do_set_bitrate(). Bail out if neither
* is given.
*/
- if (!priv->data_bittiming_const && !priv->do_set_data_bittiming)
+ 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,
- priv->data_bittiming_const,
- priv->data_bitrate_const,
- priv->data_bitrate_const_cnt);
+ 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);
+ priv.bitrate_max);
return -EINVAL;
}
- memcpy(&priv->data_bittiming, &dbt, sizeof(dbt));
-
can_calc_tdco(dev);
- if (priv->do_set_data_bittiming) {
+ if (priv.do_set_data_bittiming) {
/* Finally, set the bit-timing registers */
- err = priv->do_set_data_bittiming(dev);
+ err = priv.do_set_data_bittiming(dev);
if (err)
return err;
}
@@ -201,28 +203,30 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
if (data[IFLA_CAN_TERMINATION]) {
const u16 termval = nla_get_u16(data[IFLA_CAN_TERMINATION]);
- const unsigned int num_term = priv->termination_const_cnt;
+ const unsigned int num_term = priv.termination_const_cnt;
unsigned int i;
- if (!priv->do_set_termination)
+ if (!priv.do_set_termination)
return -EOPNOTSUPP;
/* check whether given value is supported by the interface */
for (i = 0; i < num_term; i++) {
- if (termval == priv->termination_const[i])
+ if (termval == priv.termination_const[i])
break;
}
if (i >= num_term)
return -EINVAL;
/* Finally, set the termination value */
- err = priv->do_set_termination(dev, termval);
+ err = priv.do_set_termination(dev, termval);
if (err)
return err;
- priv->termination = termval;
+ priv.termination = termval;
}
+ memcpy(netdev_priv(dev), &priv, sizeof(priv));
+
return 0;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] can: netlink: prevent incoherent can configuration in case of early return
2021-08-20 9:03 [PATCH] can: netlink: prevent incoherent can configuration in case of early return Vincent Mailhol
@ 2021-08-23 2:44 ` Vincent MAILHOL
0 siblings, 0 replies; 2+ messages in thread
From: Vincent MAILHOL @ 2021-08-23 2:44 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can; +Cc: netdev, open list
Replying to myself...
On Fri. 20 Aug 2021 at 18: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 field 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.
>
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
> Hi Marc,
>
> Do you think this need a "Fixes" tag?
> If yes, feel free to add this line to the patch:
>
> Fixes: 9859ccd2c8be ("can: introduce the data bitrate configuration for CAN FD")
> ---
> drivers/net/can/dev/netlink.c | 86 ++++++++++++++++++-----------------
> 1 file changed, 45 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
> index 80425636049d..6a14c51a058b 100644
> --- a/drivers/net/can/dev/netlink.c
> +++ b/drivers/net/can/dev/netlink.c
> @@ -58,14 +58,20 @@ 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. net/core/rtnetlink.c has a global
> + * mutex so static declaration is OK
> + */
> + static struct can_priv priv;
> int err;
>
> /* We need synchronization with dev->stop() */
> ASSERT_RTNL();
>
> + memcpy(&priv, netdev_priv(dev), sizeof(priv));
> +
> 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)
> @@ -76,28 +82,26 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
> * directly via do_set_bitrate(). Bail out if neither
> * is given.
> */
> - if (!priv->bittiming_const && !priv->do_set_bittiming)
> + 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,
> - priv->bittiming_const,
> - priv->bitrate_const,
> - priv->bitrate_const_cnt);
> + 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);
> + priv.bitrate_max);
> return -EINVAL;
> }
>
> - memcpy(&priv->bittiming, &bt, sizeof(bt));
> -
> - if (priv->do_set_bittiming) {
> + if (priv.do_set_bittiming) {
> /* Finally, set the bit-timing registers */
> - err = priv->do_set_bittiming(dev);
> + err = priv.do_set_bittiming(dev);
> if (err)
> return err;
> }
> @@ -112,11 +116,11 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
> if (dev->flags & IFF_UP)
> return -EBUSY;
> cm = nla_data(data[IFLA_CAN_CTRLMODE]);
> - ctrlstatic = priv->ctrlmode_static;
> + ctrlstatic = priv.ctrlmode_static;
> maskedflags = cm->flags & cm->mask;
>
> /* check whether provided bits are allowed to be passed */
> - if (maskedflags & ~(priv->ctrlmode_supported | ctrlstatic))
> + if (maskedflags & ~(priv.ctrlmode_supported | ctrlstatic))
> return -EOPNOTSUPP;
>
> /* do not check for static fd-non-iso if 'fd' is disabled */
> @@ -128,16 +132,16 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
> return -EOPNOTSUPP;
>
> /* clear bits to be modified and copy the flag values */
> - priv->ctrlmode &= ~cm->mask;
> - priv->ctrlmode |= maskedflags;
> + priv.ctrlmode &= ~cm->mask;
> + priv.ctrlmode |= maskedflags;
>
> /* CAN_CTRLMODE_FD can only be set when driver supports FD */
> - if (priv->ctrlmode & CAN_CTRLMODE_FD) {
> + if (priv.ctrlmode & CAN_CTRLMODE_FD) {
> dev->mtu = CANFD_MTU;
> } else {
> dev->mtu = CAN_MTU;
> - memset(&priv->data_bittiming, 0,
> - sizeof(priv->data_bittiming));
> + memset(&priv.data_bittiming, 0,
> + sizeof(priv.data_bittiming));
> }
> }
>
> @@ -145,7 +149,7 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
> /* Do not allow changing restart delay while running */
> if (dev->flags & IFF_UP)
> return -EBUSY;
> - priv->restart_ms = nla_get_u32(data[IFLA_CAN_RESTART_MS]);
> + priv.restart_ms = nla_get_u32(data[IFLA_CAN_RESTART_MS]);
> }
>
> if (data[IFLA_CAN_RESTART]) {
> @@ -158,7 +162,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)
> @@ -169,31 +173,29 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
> * directly via do_set_bitrate(). Bail out if neither
> * is given.
> */
> - if (!priv->data_bittiming_const && !priv->do_set_data_bittiming)
> + 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,
> - priv->data_bittiming_const,
> - priv->data_bitrate_const,
> - priv->data_bitrate_const_cnt);
> + 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);
> + priv.bitrate_max);
> return -EINVAL;
> }
>
> - memcpy(&priv->data_bittiming, &dbt, sizeof(dbt));
> -
> can_calc_tdco(dev);
can_calc_tdco() retrieves can_priv from the network device and
directly writes its changes to it. I will send a v2 in which I
will change the prototype of can_calc_tdco() so that the changes
are written to the temporary priv instead.
> - if (priv->do_set_data_bittiming) {
> + if (priv.do_set_data_bittiming) {
> /* Finally, set the bit-timing registers */
> - err = priv->do_set_data_bittiming(dev);
> + err = priv.do_set_data_bittiming(dev);
> if (err)
> return err;
> }
> @@ -201,28 +203,30 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
>
> if (data[IFLA_CAN_TERMINATION]) {
> const u16 termval = nla_get_u16(data[IFLA_CAN_TERMINATION]);
> - const unsigned int num_term = priv->termination_const_cnt;
> + const unsigned int num_term = priv.termination_const_cnt;
> unsigned int i;
>
> - if (!priv->do_set_termination)
> + if (!priv.do_set_termination)
> return -EOPNOTSUPP;
>
> /* check whether given value is supported by the interface */
> for (i = 0; i < num_term; i++) {
> - if (termval == priv->termination_const[i])
> + if (termval == priv.termination_const[i])
> break;
> }
> if (i >= num_term)
> return -EINVAL;
>
> /* Finally, set the termination value */
> - err = priv->do_set_termination(dev, termval);
> + err = priv.do_set_termination(dev, termval);
> if (err)
> return err;
>
> - priv->termination = termval;
> + priv.termination = termval;
> }
>
> + memcpy(netdev_priv(dev), &priv, sizeof(priv));
> +
> return 0;
> }
Yours sincerely,
Vincent Mailhol
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-08-23 2:45 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-20 9:03 [PATCH] can: netlink: prevent incoherent can configuration in case of early return Vincent Mailhol
2021-08-23 2:44 ` Vincent MAILHOL
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).