linux-kernel - Re: [PATCH] ptmx: adding handshake support

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:	Sun, 30 Mar 2008 14:48:04 +0200
From:	sander van ginkel <sander@...-ginkel.eu>
To:	Alan Cox <alan@...rguk.ukuu.org.uk>
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ptmx: adding handshake support

Ok, I will test this kernel if its stable enough for my needs, and  
later this week I will move on to the latest kernel.
Further I shall look at your comments and make some corrections to my code.

One thing thats comming up to me:

>> +    case 0x5426: /* Set all of the handshake line, even the  
>> normally  read only */
>>

> Use a proper value as I said before.


I may pick the last value + 1 defined in the lib?

thanks for your input


Quoting Alan Cox <alan@...rguk.ukuu.org.uk>:

> On Sun, 30 Mar 2008 14:09:48 +0200
> postmaster@...-ginkel.eu wrote:
>
>> This weekend I upgraded my kernel to 2.6.22.5 and tried my patch.
>> Unfortunately pty.c seems to be a slightly different, so generated a new
>> patch:
>
> Really you want to be working against 2.6.25-rc5-mm1 or later as there
> are big changes in the tty layer pending, some of which affect the
> locking on stuff like this.
>
>> +struct pty_mcrmsr {
>> +
>> +	struct semaphore        sem;            /* locks this structure */
>
> You create a lock but don't use it. Also btw the -mm tty layer has a
> ctrl_lock you could use instead.
>
>> +	/* for tiocmget and tiocmset functions */
>> +
>> +	int                     msr;            /* MSR shadow */
>> +	int                     mcr;            /* MCR shadow */
>
> Could be one value using the existing bit masks.
>
>> +	mcrmsr = kmalloc(sizeof(*mcrmsr), GFP_KERNEL);
>> +
>> +	init_MUTEX(&mcrmsr->sem);
>
> Crashes on allocation failure
>
>> +	if (tty->driver_data != NULL) {
>> +		msr = mcrmsr->msr;
>> +		mcr = mcrmsr->mcr;
>> +	}
>
> Always true
>
>> +
>> +	result = ((mcr & 0x01)  ? TIOCM_DTR  : 0) |  /* DTR is set */
>> +		((mcr & 0x02)  ? TIOCM_RTS  : 0) |  /* RTS is set */
>> +		((mcr & 0x04)  ? TIOCM_LOOP : 0) |  /* LOOP is set */
>> +		((msr & 0x08)  ? TIOCM_CTS  : 0) |  /* CTS is set */
>> +		((msr & 0x10)  ? TIOCM_CAR  : 0) |  /* Carrier detect is set*/
>> +		((msr & 0x20)  ? TIOCM_RI   : 0) |  /* Ring Indicator is set */
>> +		((msr & 0x40)  ? TIOCM_DSR  : 0);   /* DSR is set */
>
> Use the mcr/msr bits as is and you don't need this mess
>
>> +	if (set & TIOCM_DTR)
>> +		mcr |= 0x01;
>> +	if (set & TIOCM_RTS)
>> +		mcr |= 0x02;
>> +	if (set & TIOCM_LOOP)
>
> Ditto
>
>> +	/* set the new MCR value in the device */
>> +
>> +	if (tty->driver_data != NULL)
>> +		mcrmsr->mcr=mcr;
>
> That may cause wakeups, open, hangup etc to occur - does that need to be
> considered here.
>
>> +	case 0x5426: /* Set all of the handshake line, even the normally
>> read only */
>
> Use a proper value as I said before.
>
>
>
>



--
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/