Hi Oliver, On Wed. 8 sep. 2021 at 20:41, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> 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@xxxxxxxxxx> 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
