Re: [PATCH v3] can, tty: elmcan CAN/ldisc driver for ELM327 based OBD-II adapters

[Marc Kleine-Budde]

On 17.03.2022 21:18:22, Max Staudt wrote:
> > > +/* Bits in elm->cmds_todo */
> > > +enum ELM_TODO {  
> >         ^^^^^^^^
> > small caps please, and Vincent alreadt commented on the name.
> 
> Small caps? Sorry, that's not possible in plain ASCII.
> You probably mean something else, but I'm not sure what?

I meant to say lowercase, sorry for the confusion.

[...]

> > > +	/* Regular parsing */
> > > +	switch (elm->state) {
> > > +	case ELM_RECEIVING:
> > > +		if (elm327_parse_frame(elm, len)) {
> > > +			/* Parse an error line. */
> > > +			elm327_parse_error(elm, len);
> > > +
> > > +			/* Start afresh. */
> > > +			elm327_kick_into_cmd_mode(elm);
> > > +		}
> > > +		break;
> > > +	default:
> > > +		break;
> > > +	}
> > > +}
> > > +
> > > +/* Assumes elm->lock taken. */
> > > +static void elm327_handle_prompt(struct elmcan *elm)
> > > +{
> > > +	struct can_frame *frame = &elm->can_frame;
> > > +	char local_txbuf[20];  
> > 
> > How can you be sure, that the local_txbuf is large enough?
> 
> It's filled in this very same function, with sprintf() or a strcpy()
> from one of the short strings in elm327_init_script (see next quote
> below). I've calculated the maximum length that can occur out of all
> these possibilities in the current code, and set that as the length of
> local_txbuf.

You can use something like "local_txbuf[sizeof("ATZ;ATDT0815;ATH")]"
with the longest ATZ command you can produce here.

> > > +	/* Reconfigure ELM327 step by step as indicated by
> > > elm->cmds_todo */
> > > +	if (test_bit(TODO_INIT, &elm->cmds_todo)) {
> > > +		strcpy(local_txbuf, *elm->next_init_cmd);  
> > 
> > strncpy()
> 
> For this, there would have to be an entry in elm327_init_script that is
> longer than sizeof(local_txbuf) - 1. I highly doubt there ever will be,
> and even if someone does come up with one (maybe a huge new command in a
> future ELM327 revision), then strncpy would silently cut off the end and
> induce unexpected failure. Most importantly, this failure would be
> silent - the driver doesn't check the ELM's responses by design!
> 
> I suggest an assert here. How about something like this?
> 
> 	if (strlen(*elm->next_init_cmd) < sizeof(local_txbuf))
> 		strcpy(local_txbuf, *elm->next_init_cmd);
> 	else
> 		WARN_ONCE(...)
>
> If elm327_init_script contains an item longer than this buffer, then
> the buffer size needs to be increased. Simple programming error IMHO.
> I'd also add a comment to state this, next to elm327_init_script.
> 
> What do you think?

You can use BUILD_BUG_ON() (see linux/build_bug.h) inside your C
function to make a compile time check, or static_assert() outside of C
functions.

> > > +	} else if (test_and_clear_bit(TODO_SILENT_MONITOR,
> > > &elm->cmds_todo)) {
> > > +		sprintf(local_txbuf, "ATCSM%i\r",
> > > +			!(!(elm->can.ctrlmode &
> > > CAN_CTRLMODE_LISTENONLY)));  
> > 
> > snprintf()
> 
> See above. This size is predictable, and used to size local_txbuf.
> 
> Thinking about it, since this size is easily predictable, the compiler
> could also do it, and that would turn snprintf() into a compile time
> check.
> 
> Unfortunately I couldn't make GCC shout at me for giving snprintf() too
> small a buffer to fit all possible expansions of this format string. Is
> this even possible?

In user space, I've seen warnings like that, not sure about the kernel.

> > > +static int elmcan_netdev_open(struct net_device *dev)
> > > +{
> > > +	struct elmcan *elm = netdev_priv(dev);
> > > +	int err;
> > > +
> > > +	spin_lock_bh(&elm->lock);
> > > +	if (elm->hw_failure) {
> > > +		netdev_err(elm->dev, "Refusing to open interface
> > > after a hardware fault has been detected.\n");
> > > +		spin_unlock_bh(&elm->lock);
> > > +		return -EIO;
> > > +	}  
> > 
> > How to recover from this error?
> 
> The user can detach and reattach the ldisc as often as desired.
> 
> There is currently no intention to recover automatically. Once
> elm->hw_failure is set, something really weird must have happened such
> as unexpected characters on the UART. Since these devices are usually a
> PIC right next to a UART-USB bridge chip, which is why I deem this
> indicative of hardware too faulty to be trusted in any way.
> 
> Regular "expected" errors are parsed and dealt with by sending error
> frames in elm327_parse_error(). These do not trigger hw_failure.

Ok, in other drivers I usually do a full reset during an ifdown/ifup
cycle....at least for non hot plug-able devices.

> > > +	elm->txbuf = kmalloc(ELM327_SIZE_TXBUF, GFP_KERNEL);  
> > 
> > Why do you allocate an extra buffer?
> 
> If I remember correctly, I was told that this is preferred because
> drivers can DMA out of the aligned buffer. I didn't question that. I
> can simply allocate a buffer as part of struct elmcan if you prefer.

You can force proper alignment with marking the memory as
____cacheline_aligned. Extra bonus for checking (and optimizing)
structure packing with the "pahole" tool.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

Attachment: signature.asc
Description: PGP signature