linux-kernel - Re: [PATCH v2 09/19] staging/speakup: Use tty_ldisc

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, 10 Jan 2016 21:40:11 -0800
From:	Peter Hurley <peter@...leysoftware.com>
To:	Ben Hutchings <ben@...adent.org.uk>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:	Jiri Slaby <jslaby@...e.cz>, linux-kernel@...r.kernel.org,
	stable@...r.kernel.org
Subject: Re: [PATCH v2 09/19] staging/speakup: Use tty_ldisc_ref() for paste
 kworker

On 01/10/2016 04:25 PM, Peter Hurley wrote:
> On 01/10/2016 03:16 PM, Ben Hutchings wrote:
>> On Sat, 2016-01-09 at 20:41 -0800, Peter Hurley wrote:
>>> As the function documentation for tty_ldisc_ref_wait() notes, it is
>>> only callable from a tty file_operations routine; otherwise there
>>> is no guarantee the ref won't be NULL.
>>>
>>> The key difference with the VT's paste_selection() is that is an ioctl,
>>> where __speakup_paste_selection() is completely asynch kworker, kicked
>>> off from interrupt context.
>>>
>>> Fixes: 28a821c30688 ("Staging: speakup: Update __speakup_paste_selection()
>>>        tty (ab)usage to match vt")
>>> Cc: <stable@...r.kernel.org>
>>>
>>> Signed-off-by: Peter Hurley <peter@...leysoftware.com>
>>> ---
>>>  drivers/staging/speakup/selection.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/staging/speakup/selection.c b/drivers/staging/speakup/selection.c
>>> index aa5ab6c..86c0b9a 100644
>>> --- a/drivers/staging/speakup/selection.c
>>> +++ b/drivers/staging/speakup/selection.c
>>> @@ -142,7 +142,9 @@ static void __speakup_paste_selection(struct work_struct *work)
>>>  	struct tty_ldisc *ld;
>>>  	DECLARE_WAITQUEUE(wait, current);
>>>  
>>> -	ld = tty_ldisc_ref_wait(tty);
>>> +	ld = tty_ldisc_ref(tty);
>>> +	if (!ld)
>>> +		return;
>>>  	tty_buffer_lock_exclusive(&vc->port);
>>>  
>>>  	add_wait_queue(&vc->paste_wait, &wait);
>>
>> This leaks a reference to the tty.  Instead of returning directly, I
>> think you need to add a label and goto the tty_kref_put() at the bottom
>> of the function.
> 
> Ugh, speakup_paste_selection() is a worse hack than I thought it was.

What if the kworker has already been scheduled but not run? Leaky reference
anyway.

What guarantees that the kref is gettable to begin with and isn't incrementing
from 0?

This isn't how tty krefs work.

I'll fix the patch to drop the kref but this is broken anyway.

Regards,
Peter Hurley