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

From: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
To: Oliver Hartkopp <socketcan@hartkopp.net>
Cc: Marc Kleine-Budde <mkl@pengutronix.de>,
	linux-can <linux-can@vger.kernel.org>
Subject: Re: [PATCH v3 1/2] can: netlink: prevent incoherent can configuration in case of early return
Date: Tue, 14 Sep 2021 18:35:31 +0900	[thread overview]
Message-ID: <CAMZ6RqKpiYOLn7=NoN1U4PwnW4fdrJ1N57nGrCJ6FgKnyx5GvQ@mail.gmail.com> (raw)
In-Reply-To: <bccb03f4-5179-895a-f803-5adf543a8c19@hartkopp.net>

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