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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 15 Mar 2022 11:21:35 +0100
From:   Marc Kleine-Budde <mkl@...gutronix.de>
To:     Max Staudt <max@...as.org>
Cc:     Vincent Mailhol <vincent.mailhol@...il.com>,
        Wolfgang Grandegger <wg@...ndegger.com>,
        linux-can@...r.kernel.org, Oliver Neukum <oneukum@...e.com>,
        linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jslaby@...e.com>
Subject: Re: [PATCH v3] can, tty: elmcan CAN/ldisc driver for ELM327 based
 OBD-II adapters

On 12.03.2022 22:21:42, Max Staudt wrote:
> @Vincent - two more things have remained, and I hope it's okay once I
> explain them:
> 
> 1. _memstrcmp() - memcmp() vs. str(n)cmp()
> 
> The _memstrcmp() function does not compare strings, it compares raw
> buffers. I am just using C strings for the fixed buffers to compare
> against, as that allows for shorter and easier to read code. The NUL
> byte at the end of those strings goes unused.
> 
> Also, I have not looked at the assembly produced, since the semantics
> are different: str(n)cmp() needs to look for NUL bytes in the buffer(s),
> which is unnecessary here. As a bonus, NUL will never even occur
> because my code filters those bytes out upon reception from the UART
> (it's a documented quirk of the ELM327).
> 
> Finally, even if I were to use strcmp(), the code would still look just
> as ugly. Except the machine would also look for NUL bytes, and the next
> human to read the code would wonder why I'm comparing strings and not
> buffers.
> 
> Hence memcmp(), to help the code self-document and the compiler
> optimise - I hope that's okay.

Looking at the code:

> +/* Compare a buffer to a fixed string */
> +static inline int _memstrcmp(const u8 *mem, const char *str)
> +{
> +     return memcmp(mem, str, strlen(str));

The _memstrcmp is sometimes directly used. Where's the check that mem is
valid for strlen(len)? Better only use _len_memstrcmp().

> +}
> +
> +/* Compare buffer to string length, then compare buffer to fixed string.
> + * This ensures two things:
> + *  - It flags cases where the fixed string is only the start of the
> + *    buffer, rather than exactly all of it.
> + *  - It avoids byte comparisons in case the length doesn't match.
> + */
> +static inline int _len_memstrcmp(const u8 *mem, size_t mem_len, const char *str)

make it a bool.

> +{
> + 	size_t str_len = strlen(str);
> +
> +	return (mem_len != str_len) || memcmp(mem, str, str_len);
> +}

Can you rename _len_memstrcmp into something like elm327_match() or so?

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 |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)