linux-kernel - Re: [PATCH 2/2] misc: add real function open/read/write/ioctl/close for moxa_serial

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:	Mon, 25 Apr 2011 12:40:49 +0100
From:	Alan Cox <alan@...rguk.ukuu.org.uk>
To:	Jimmy Chen (陳永達) <jimmy.chen@...a.com>
Cc:	<linux-kernel@...r.kernel.org>, <gregkh@...e.de>
Subject: Re: [PATCH 2/2] misc: add real function open/read/write/ioctl/close
 for moxa_serial_io driver

> Add real function and GPL license.

We really don't need an entire extra copy of the GPL. There is one in the
source tree already to which you can refer!

In code terms:
	- We have an expected interface for RS485/RS232 switching which
	  is in the serial drivers. So the functionality is good but it
	  really wants connecting to the serial driver.



> -/* write function called when to /dev/mxsio is written */
> -static ssize_t io_write (struct file *file, const char *buf,
> -		size_t count, loff_t *ppos) {
> -	int err;
> -	err = copy_from_user(string,buf,count);
> -
> -	if(count < 3)
> -		return -EINVAL;
> -	if (err != 0)
> -		return -EFAULT;
> -
> -	outb((unsigned char)string[2], (((unsigned short)string[0])<<8)|((unsigned short)string[1]));

This can access anything so is entirely unsafe and unsuitable. Also you
have no locking but use static buffers.

> -		outb(do_state_keep,BASEPORT+5);

BASEPORT appears to be a magic constant - where does it come from given
0x8000 is usually in PCI space ?

> -	outb(do_state_keep, BASEPORT+5);
> -	outb(0x00, BASEPORT);
> -	
> -	/* set default serial mode to RS232 */
> -	outb(0x88, BASEPORT+4);

No checks to see if the relevant hardware is really present - so it will
crash other systems

> -void superio_enter_config(void) {
> -#if defined(DA681)
> -	outb (0x87, SUPERIO_CONFIG_PORT);
> -	outb(0x80, 0xeb); // a Small delay
> -	outb (0x87, SUPERIO_CONFIG_PORT);
> -	outb(0x80, 0xeb); // a Small delay
> -#elif defined(V21XX)
> -	outb (0x87, SUPERIO_CONFIG_PORT);
> -	outb (0x01, SUPERIO_CONFIG_PORT);
> -	outb (0x55, SUPERIO_CONFIG_PORT);
> -	outb (0x55, SUPERIO_CONFIG_PORT);
> -#endif

And what if a kernel is being built for two different configs at once ?

> -}
> -
> -void superio_exit_config(void) {
> -	outb(0x02, SUPERIO_CONFIG_PORT);
> -        outb( 0x80, 0xeb); // a Small delay

Use outb_p()

the kernel then will figure out the right way to do the delay. Poking
random I/O ports won't do what you want on some systems and may even
crash them.
--
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/