linux-kernel - Re: [PATCH] Staging: speakup: Fix getting port information

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]

Message-ID: <20160104122005.GI5284@mwanda>
Date:	Mon, 4 Jan 2016 15:20:05 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Samuel Thibault <samuel.thibault@...-lyon.org>,
	Chen Gang <gang.chen@...anux.com>,
	William Hubbs <w.d.hubbs@...il.com>,
	Chris Brannon <chris@...-brannons.com>,
	Kirk Reiser <kirk@...sers.ca>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	speakup@...ux-speakup.org, devel@...verdev.osuosl.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Staging: speakup: Fix getting port information

On Sun, Jan 03, 2016 at 12:25:29AM +0100, Samuel Thibault wrote:
> 5e6dc54 broke the port information in the speakup driver:

There is a correct format for this.

Patch 5e6dc548e453 ('drivers: staging: speakup: serialio: only use
platform specific SERIAL_PORT_DFNS.') broke the port information ...

If you specify fewer than 12 numbers from the git hash it might not be
unique next year.  If you leave out the patch title then no one
knows what you are talking about because we are not robots and we are
better at remembering text instead if hex numbers.  Also CC the guilty
party instead of discussing them behind their backs.

> SERIAL_PORT_DFNS only gets defined if asm/serial.h is included.

No, that's not true.  There is a #define SERIAL_PORT_DFN at the start of
the file.  I am confused.

> 
> Along the way, make sure that we do have information for the requested
> serial port number (index)
> 
> Signed-off-by: Samuel Thibault <samuel.thibault@...-lyon.org>
> 
> --- a/drivers/staging/speakup/serialio.c
> +++ b/drivers/staging/speakup/serialio.c
> @@ -6,6 +6,9 @@
>  #include "spk_priv.h"
>  #include "serialio.h"
>  
> +#include <linux/serial_core.h>
> +#include <asm/serial.h>

This should be: <linux/serial.h> probably.

> +
>  #ifndef SERIAL_PORT_DFNS
>  #define SERIAL_PORT_DFNS
>  #endif
> @@ -26,6 +29,11 @@ const struct old_serial_port *spk_serial
>  	const struct old_serial_port *ser = rs_table + index;
>  	int err;
>  
> +	if (index > sizeof(rs_table) / sizeof(*rs_table)) {

This has an off-by-one bug > vs >=.  Also use the ARRAY_SIZE() macro.

	if (index >= ARRAY_SIZE(rs_table)) {

Could you move the use of index below the check?  Current static
analysis tools are deficient and prefer "check first and then use" order.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/