| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 7 Sep 2021 01:03:09 +0900
From: Vincent Mailhol <mailhol.vincent@...adoo.fr>
To: Marc Kleine-Budde <mkl@...gutronix.de>, linux-can@...r.kernel.org
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
Vincent Mailhol <mailhol.vincent@...adoo.fr>
Subject: [PATCH v3 1/2] can: netlink: prevent incoherent can configuration in case of early return
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@...adoo.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
Powered by blists - more mailing lists