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 20:45:00 +0900	[thread overview]
Message-ID: <CAMZ6RqJa1+nNyoaGE7Phs3YSAMKEpYC8m=Ho7W3_XwcWdxdu5w@mail.gmail.com> (raw)
In-Reply-To: <CAMZ6RqKpiYOLn7=NoN1U4PwnW4fdrJ1N57nGrCJ6FgKnyx5GvQ@mail.gmail.com>

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