* [PATCH v3 0/2] prevent incoherent can configuration in case of early return in the CAN netlink interface
@ 2021-09-06 16:03 Vincent Mailhol
2021-09-06 16:03 ` [PATCH v3 1/2] can: netlink: prevent incoherent can configuration in case of early return Vincent Mailhol
2021-09-06 16:03 ` [PATCH v3 2/2] can: bittiming: change can_calc_tdco()'s prototype to not directly modify priv Vincent Mailhol
0 siblings, 2 replies; 9+ messages in thread
From: Vincent Mailhol @ 2021-09-06 16:03 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can; +Cc: netdev, linux-kernel, 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 1/2] can: netlink: prevent incoherent can configuration in case of early return
2021-09-06 16:03 [PATCH v3 0/2] prevent incoherent can configuration in case of early return in the CAN netlink interface Vincent Mailhol
@ 2021-09-06 16:03 ` Vincent Mailhol
2021-09-07 2:05 ` Vincent MAILHOL
2021-09-07 12:51 ` Vincent MAILHOL
2021-09-06 16:03 ` [PATCH v3 2/2] can: bittiming: change can_calc_tdco()'s prototype to not directly modify priv Vincent Mailhol
1 sibling, 2 replies; 9+ messages in thread
From: Vincent Mailhol @ 2021-09-06 16: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 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 2/2] can: bittiming: change can_calc_tdco()'s prototype to not directly modify priv
2021-09-06 16:03 [PATCH v3 0/2] prevent incoherent can configuration in case of early return in the CAN netlink interface Vincent Mailhol
2021-09-06 16:03 ` [PATCH v3 1/2] can: netlink: prevent incoherent can configuration in case of early return Vincent Mailhol
@ 2021-09-06 16:03 ` Vincent Mailhol
1 sibling, 0 replies; 9+ messages in thread
From: Vincent Mailhol @ 2021-09-06 16:03 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can; +Cc: netdev, linux-kernel, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] can: netlink: prevent incoherent can configuration in case of early return
2021-09-06 16:03 ` [PATCH v3 1/2] can: netlink: prevent incoherent can configuration in case of early return Vincent Mailhol
@ 2021-09-07 2:05 ` Vincent MAILHOL
2021-09-07 12:51 ` Vincent MAILHOL
1 sibling, 0 replies; 9+ messages in thread
From: Vincent MAILHOL @ 2021-09-07 2:05 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can; +Cc: netdev, open list
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] can: netlink: prevent incoherent can configuration in case of early return
2021-09-06 16:03 ` [PATCH v3 1/2] can: netlink: prevent incoherent can configuration in case of early return Vincent Mailhol
2021-09-07 2:05 ` Vincent MAILHOL
@ 2021-09-07 12:51 ` Vincent MAILHOL
2021-09-08 11:41 ` Oliver Hartkopp
1 sibling, 1 reply; 9+ messages in thread
From: Vincent MAILHOL @ 2021-09-07 12:51 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can; +Cc: netdev, open list
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] can: netlink: prevent incoherent can configuration in case of early return
2021-09-07 12:51 ` Vincent MAILHOL
@ 2021-09-08 11:41 ` Oliver Hartkopp
2021-09-14 9:35 ` Vincent MAILHOL
0 siblings, 1 reply; 9+ messages in thread
From: Oliver Hartkopp @ 2021-09-08 11:41 UTC (permalink / raw)
To: Vincent MAILHOL, Marc Kleine-Budde; +Cc: linux-can
- nextdev ML
- linux-kernel ML
Hi Vincent,
On 07.09.21 14:51, Vincent MAILHOL wrote:
> 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.
When the ip configuration fails you get an error code. And you
*typically* do it again to fix your wrong command line parameters.
¯\_(ツ)_/¯
If not the attempt to set the CAN interface to 'up' will fail (as the
last line of defense).
The code with all the sanity checks is already pretty complex IMO.
I wonder if this effort is worth it.
Best regards,
Oliver
>>
>> * 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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] can: netlink: prevent incoherent can configuration in case of early return
2021-09-08 11:41 ` Oliver Hartkopp
@ 2021-09-14 9:35 ` Vincent MAILHOL
2021-09-14 11:45 ` Vincent MAILHOL
0 siblings, 1 reply; 9+ messages in thread
From: Vincent MAILHOL @ 2021-09-14 9:35 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: Marc Kleine-Budde, linux-can
Hi Oliver,
On Wed. 8 sep. 2021 at 20:41, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> - nextdev ML
> - linux-kernel ML
>
> Hi Vincent,
>
> On 07.09.21 14:51, Vincent MAILHOL wrote:
> > 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.
>
> When the ip configuration fails you get an error code. And you
> *typically* do it again to fix your wrong command line parameters.
>
> ¯\_(ツ)_/¯
Overall yes. I tried to think of a counterexample and the best I
could think of is if the user does:
# ip link set can0 type can bitrate 500000 dbitrate 20000000 fd on; ip
link set can0 up
In which case, the .ndo_open() function of the driver would be
triggered with incorrect parameters.
> If not the attempt to set the CAN interface to 'up' will fail (as the
> last line of defense).
Mostly correct: open_candev() will spot that the data bitrate is not set
making the .ndo_open() fails as long as the driver correctly
checks open_candev() return value.
However, one driver fails to check the return value of open_candev():
https://elixir.bootlin.com/linux/v5.11/source/drivers/net/can/softing/softing_fw.c#L636
So, for this particular driver, we can send incoherent values to the device.
> The code with all the sanity checks is already pretty complex IMO.
ACK.
> I wonder if this effort is worth it.
Well, I was thinking "this is a bug so let's fix it". But your
argument is fair. I also did not like how complex the code was
getting when trying to fix that. I guess that this bug is
acceptable. I will leave it as it is.
Now, I am just worried about the softing driver.
Thanks.
Yours sincerely,
Vincent
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] can: netlink: prevent incoherent can configuration in case of early return
2021-09-14 9:35 ` Vincent MAILHOL
@ 2021-09-14 11:45 ` Vincent MAILHOL
2021-09-14 12:13 ` Oliver Hartkopp
0 siblings, 1 reply; 9+ messages in thread
From: Vincent MAILHOL @ 2021-09-14 11:45 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: Marc Kleine-Budde, linux-can
On Tue. 14 Sep. 2021 at 18:35, Vincent MAILHOL
<mailhol.vincent@wanadoo.fr> wrote:
> Hi Oliver,
>
> On Wed. 8 sep. 2021 at 20:41, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> > - nextdev ML
> > - linux-kernel ML
> >
> > Hi Vincent,
> >
> > On 07.09.21 14:51, Vincent MAILHOL wrote:
> > > 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.
> >
> > When the ip configuration fails you get an error code. And you
> > *typically* do it again to fix your wrong command line parameters.
> >
> > ¯\_(ツ)_/¯
>
> Overall yes. I tried to think of a counterexample and the best I
> could think of is if the user does:
> # ip link set can0 type can bitrate 500000 dbitrate 20000000 fd on; ip
> link set can0 up
>
> In which case, the .ndo_open() function of the driver would be
> triggered with incorrect parameters.
>
> > If not the attempt to set the CAN interface to 'up' will fail (as the
> > last line of defense).
>
> Mostly correct: open_candev() will spot that the data bitrate is not set
> making the .ndo_open() fails as long as the driver correctly
> checks open_candev() return value.
>
> However, one driver fails to check the return value of open_candev():
> https://elixir.bootlin.com/linux/v5.11/source/drivers/net/can/softing/softing_fw.c#L636
>
> So, for this particular driver, we can send incoherent values to the device.
>
> > The code with all the sanity checks is already pretty complex IMO.
>
> ACK.
>
> > I wonder if this effort is worth it.
>
> Well, I was thinking "this is a bug so let's fix it". But your
> argument is fair. I also did not like how complex the code was
> getting when trying to fix that. I guess that this bug is
> acceptable. I will leave it as it is.
>
> Now, I am just worried about the softing driver.
Actually, the softing driver is not CAN-FD capable.
So there was probably no real needs to worry.
> Thanks.
>
>
> Yours sincerely,
> Vincent
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] can: netlink: prevent incoherent can configuration in case of early return
2021-09-14 11:45 ` Vincent MAILHOL
@ 2021-09-14 12:13 ` Oliver Hartkopp
0 siblings, 0 replies; 9+ messages in thread
From: Oliver Hartkopp @ 2021-09-14 12:13 UTC (permalink / raw)
To: Vincent MAILHOL; +Cc: Marc Kleine-Budde, linux-can
On 14.09.21 13:45, Vincent MAILHOL wrote:
>> However, one driver fails to check the return value of open_candev():
>> https://elixir.bootlin.com/linux/v5.11/source/drivers/net/can/softing/softing_fw.c#L636
>>
>> So, for this particular driver, we can send incoherent values to the device.
>>
>>> The code with all the sanity checks is already pretty complex IMO.
>>
>> ACK.
>>
>>> I wonder if this effort is worth it.
>>
>> Well, I was thinking "this is a bug so let's fix it". But your
>> argument is fair. I also did not like how complex the code was
>> getting when trying to fix that. I guess that this bug is
>> acceptable. I will leave it as it is.
>>
>> Now, I am just worried about the softing driver.
IMO this should not be a problem.
The code is only used for a *restart* of the CAN interface. Therefore
the bitrates have been checked at the original (first) start.
> Actually, the softing driver is not CAN-FD capable.
> So there was probably no real needs to worry.
Yes. I luckily checked the other mails in the inbox before answering
that ;-)
Best,
Oliver
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-09-14 12:13 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-06 16:03 [PATCH v3 0/2] prevent incoherent can configuration in case of early return in the CAN netlink interface Vincent Mailhol
2021-09-06 16:03 ` [PATCH v3 1/2] can: netlink: prevent incoherent can configuration in case of early return Vincent Mailhol
2021-09-07 2:05 ` Vincent MAILHOL
2021-09-07 12:51 ` Vincent MAILHOL
2021-09-08 11:41 ` Oliver Hartkopp
2021-09-14 9:35 ` Vincent MAILHOL
2021-09-14 11:45 ` Vincent MAILHOL
2021-09-14 12:13 ` Oliver Hartkopp
2021-09-06 16:03 ` [PATCH v3 2/2] can: bittiming: change can_calc_tdco()'s prototype to not directly modify priv 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).