linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] tty_io: Fix a missing-check bug in drivers/tty/tty_io.c
@ 2019-05-22  1:40 Gen Zhang
  2019-05-22  4:25 ` Jiri Slaby
  0 siblings, 1 reply; 10+ messages in thread
From: Gen Zhang @ 2019-05-22  1:40 UTC (permalink / raw)
  To: jslaby; +Cc: linux-kernel

In alloc_tty_struct(), tty->dev is assigned by tty_get_device(). And it
calls class_find_device(). And class_find_device() may return NULL.
And tty->dev is dereferenced in the following codes. When 
tty_get_device() returns NULL, dereferencing this tty->dev null pointer
may cause the kernel go wrong. Thus we should check tty->dev.
Further, if tty_get_device() returns NULL, we should free tty and 
return NULL.

Signed-off-by: Gen Zhang <blackgod016574@gmail.com>

---
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 033ac7e..1444b59 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -3008,6 +3008,10 @@ struct tty_struct *alloc_tty_struct(struct tty_driver *driver, int idx)
 	tty->index = idx;
 	tty_line_name(driver, idx, tty->name);
 	tty->dev = tty_get_device(tty);
+	if (!tty->dev) {
+		kfree(tty);
+		return NULL;
+	}
 
 	return tty;
 }

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] tty_io: Fix a missing-check bug in drivers/tty/tty_io.c
  2019-05-22  1:40 [PATCH] tty_io: Fix a missing-check bug in drivers/tty/tty_io.c Gen Zhang
@ 2019-05-22  4:25 ` Jiri Slaby
  2019-05-22  8:06   ` Gen Zhang
  0 siblings, 1 reply; 10+ messages in thread
From: Jiri Slaby @ 2019-05-22  4:25 UTC (permalink / raw)
  To: Gen Zhang; +Cc: linux-kernel

On 22. 05. 19, 3:40, Gen Zhang wrote:
> In alloc_tty_struct(), tty->dev is assigned by tty_get_device(). And it
> calls class_find_device(). And class_find_device() may return NULL.
> And tty->dev is dereferenced in the following codes. When 
> tty_get_device() returns NULL, dereferencing this tty->dev null pointer
> may cause the kernel go wrong. Thus we should check tty->dev.
> Further, if tty_get_device() returns NULL, we should free tty and 
> return NULL.
> 
> Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
> 
> ---
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 033ac7e..1444b59 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -3008,6 +3008,10 @@ struct tty_struct *alloc_tty_struct(struct tty_driver *driver, int idx)
>  	tty->index = idx;
>  	tty_line_name(driver, idx, tty->name);
>  	tty->dev = tty_get_device(tty);
> +	if (!tty->dev) {
> +		kfree(tty);
> +		return NULL;
> +	}

This is incorrect, you introduced an ldisc reference leak.

And can this happen at all?

thanks,
-- 
js
suse labs

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] tty_io: Fix a missing-check bug in drivers/tty/tty_io.c
  2019-05-22  4:25 ` Jiri Slaby
@ 2019-05-22  8:06   ` Gen Zhang
  2019-05-22  8:15     ` Jiri Slaby
  0 siblings, 1 reply; 10+ messages in thread
From: Gen Zhang @ 2019-05-22  8:06 UTC (permalink / raw)
  To: Jiri Slaby; +Cc: linux-kernel

On Wed, May 22, 2019 at 06:25:36AM +0200, Jiri Slaby wrote:
> On 22. 05. 19, 3:40, Gen Zhang wrote:
> > In alloc_tty_struct(), tty->dev is assigned by tty_get_device(). And it
> > calls class_find_device(). And class_find_device() may return NULL.
> > And tty->dev is dereferenced in the following codes. When 
> > tty_get_device() returns NULL, dereferencing this tty->dev null pointer
> > may cause the kernel go wrong. Thus we should check tty->dev.
> > Further, if tty_get_device() returns NULL, we should free tty and 
> > return NULL.
> > 
> > Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
> > 
> > ---
> > diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> > index 033ac7e..1444b59 100644
> > --- a/drivers/tty/tty_io.c
> > +++ b/drivers/tty/tty_io.c
> > @@ -3008,6 +3008,10 @@ struct tty_struct *alloc_tty_struct(struct tty_driver *driver, int idx)
> >  	tty->index = idx;
> >  	tty_line_name(driver, idx, tty->name);
> >  	tty->dev = tty_get_device(tty);
> > +	if (!tty->dev) {
> > +		kfree(tty);
> > +		return NULL;
> > +	}
> 
> This is incorrect, you introduced an ldisc reference leak.
Thanks for your reply, Jiri!
And what do you mean by an ldisc reference leak? I did't get the reason
of introducing it.
> 
> And can this happen at all?
I think tty_get_device() may happen to return NULL. Because it calls 
class_find_device() and there's a chance class_find_device() returns
NULL.
Thanks
Gen
> 
> thanks,
> -- 
> js
> suse labs

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] tty_io: Fix a missing-check bug in drivers/tty/tty_io.c
  2019-05-22  8:06   ` Gen Zhang
@ 2019-05-22  8:15     ` Jiri Slaby
  2019-05-22 10:29       ` Johan Hovold
  2019-05-22 11:18       ` Gen Zhang
  0 siblings, 2 replies; 10+ messages in thread
From: Jiri Slaby @ 2019-05-22  8:15 UTC (permalink / raw)
  To: Gen Zhang; +Cc: linux-kernel

On 22. 05. 19, 10:06, Gen Zhang wrote:
> On Wed, May 22, 2019 at 06:25:36AM +0200, Jiri Slaby wrote:
>> On 22. 05. 19, 3:40, Gen Zhang wrote:
>>> In alloc_tty_struct(), tty->dev is assigned by tty_get_device(). And it
>>> calls class_find_device(). And class_find_device() may return NULL.
>>> And tty->dev is dereferenced in the following codes. When 
>>> tty_get_device() returns NULL, dereferencing this tty->dev null pointer
>>> may cause the kernel go wrong. Thus we should check tty->dev.
>>> Further, if tty_get_device() returns NULL, we should free tty and 
>>> return NULL.
>>>
>>> Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
>>>
>>> ---
>>> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
>>> index 033ac7e..1444b59 100644
>>> --- a/drivers/tty/tty_io.c
>>> +++ b/drivers/tty/tty_io.c
>>> @@ -3008,6 +3008,10 @@ struct tty_struct *alloc_tty_struct(struct tty_driver *driver, int idx)
>>>  	tty->index = idx;
>>>  	tty_line_name(driver, idx, tty->name);
>>>  	tty->dev = tty_get_device(tty);
>>> +	if (!tty->dev) {
>>> +		kfree(tty);
>>> +		return NULL;
>>> +	}
>>
>> This is incorrect, you introduced an ldisc reference leak.
> Thanks for your reply, Jiri!
> And what do you mean by an ldisc reference leak? I did't get the reason
> of introducing it.

Look at the top of alloc_tty_struct: there is tty_ldisc_init. If
tty_get_device fails here, you have to call tty_ldisc_deinit. Better,
you should add a failure-handling tail to this function and "goto" there.

>> And can this happen at all?
> I think tty_get_device() may happen to return NULL. Because it calls 
> class_find_device() and there's a chance class_find_device() returns
> NULL.

Sure, but can class_find_device return NULL in this tty case here?

thanks,
-- 
js
suse labs

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] tty_io: Fix a missing-check bug in drivers/tty/tty_io.c
  2019-05-22  8:15     ` Jiri Slaby
@ 2019-05-22 10:29       ` Johan Hovold
  2019-05-22 10:32         ` Jiri Slaby
  2019-05-22 11:13         ` Gen Zhang
  2019-05-22 11:18       ` Gen Zhang
  1 sibling, 2 replies; 10+ messages in thread
From: Johan Hovold @ 2019-05-22 10:29 UTC (permalink / raw)
  To: Jiri Slaby; +Cc: Gen Zhang, linux-kernel

On Wed, May 22, 2019 at 10:15:56AM +0200, Jiri Slaby wrote:
> On 22. 05. 19, 10:06, Gen Zhang wrote:
> > On Wed, May 22, 2019 at 06:25:36AM +0200, Jiri Slaby wrote:
> >> On 22. 05. 19, 3:40, Gen Zhang wrote:
> >>> In alloc_tty_struct(), tty->dev is assigned by tty_get_device(). And it
> >>> calls class_find_device(). And class_find_device() may return NULL.
> >>> And tty->dev is dereferenced in the following codes. When 
> >>> tty_get_device() returns NULL, dereferencing this tty->dev null pointer
> >>> may cause the kernel go wrong. Thus we should check tty->dev.

Where do you see that the kernel is dereferencing tty->dev without
checking for NULL first? If you can find that, then that would indeed be
a bug that needs fixing.

> >>> Further, if tty_get_device() returns NULL, we should free tty and 
> >>> return NULL.
> >>>
> >>> Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
> >>>
> >>> ---
> >>> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> >>> index 033ac7e..1444b59 100644
> >>> --- a/drivers/tty/tty_io.c
> >>> +++ b/drivers/tty/tty_io.c
> >>> @@ -3008,6 +3008,10 @@ struct tty_struct *alloc_tty_struct(struct tty_driver *driver, int idx)
> >>>  	tty->index = idx;
> >>>  	tty_line_name(driver, idx, tty->name);
> >>>  	tty->dev = tty_get_device(tty);
> >>> +	if (!tty->dev) {
> >>> +		kfree(tty);
> >>> +		return NULL;
> >>> +	}
> >>
> >> This is incorrect, you introduced an ldisc reference leak.
> > Thanks for your reply, Jiri!
> > And what do you mean by an ldisc reference leak? I did't get the reason
> > of introducing it.
> 
> Look at the top of alloc_tty_struct: there is tty_ldisc_init. If
> tty_get_device fails here, you have to call tty_ldisc_deinit. Better,
> you should add a failure-handling tail to this function and "goto" there.
> 
> >> And can this happen at all?
> > I think tty_get_device() may happen to return NULL. Because it calls 
> > class_find_device() and there's a chance class_find_device() returns
> > NULL.
> 
> Sure, but can class_find_device return NULL in this tty case here?

Yes, it can and will and that's fine, not all ttys have a struct device
(e.g. ptys). 

This patch is broken and should be dropped.

Johan

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] tty_io: Fix a missing-check bug in drivers/tty/tty_io.c
  2019-05-22 10:29       ` Johan Hovold
@ 2019-05-22 10:32         ` Jiri Slaby
  2019-05-22 11:13         ` Gen Zhang
  1 sibling, 0 replies; 10+ messages in thread
From: Jiri Slaby @ 2019-05-22 10:32 UTC (permalink / raw)
  To: Johan Hovold; +Cc: Gen Zhang, linux-kernel

On 22. 05. 19, 12:29, Johan Hovold wrote:
>> Sure, but can class_find_device return NULL in this tty case here?
> 
> Yes, it can and will and that's fine, not all ttys have a struct device
> (e.g. ptys). 

IOW, the code needs a comment, if anything.

thanks,
-- 
js
suse labs

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] tty_io: Fix a missing-check bug in drivers/tty/tty_io.c
  2019-05-22 10:29       ` Johan Hovold
  2019-05-22 10:32         ` Jiri Slaby
@ 2019-05-22 11:13         ` Gen Zhang
  2019-05-22 11:19           ` Johan Hovold
  1 sibling, 1 reply; 10+ messages in thread
From: Gen Zhang @ 2019-05-22 11:13 UTC (permalink / raw)
  To: Johan Hovold; +Cc: linux-kernel

On Wed, May 22, 2019 at 12:29:00PM +0200, Johan Hovold wrote:
> Where do you see that the kernel is dereferencing tty->dev without
> checking for NULL first? If you can find that, then that would indeed be
> a bug that needs fixing.
Thanks for your reply, Johan!
I examined the code but failed to find this situation.
Anyway, checking return value of tty_get_device() is theoritically
right. But tty->dev is never dereferenced, so checking is not needed.
However, what if in later kernels tty->dev is dereferenced by some
codes? Is it better to apply this check for this reason?
Thanks
Gen

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] tty_io: Fix a missing-check bug in drivers/tty/tty_io.c
  2019-05-22  8:15     ` Jiri Slaby
  2019-05-22 10:29       ` Johan Hovold
@ 2019-05-22 11:18       ` Gen Zhang
  1 sibling, 0 replies; 10+ messages in thread
From: Gen Zhang @ 2019-05-22 11:18 UTC (permalink / raw)
  To: Jiri Slaby; +Cc: linux-kernel

On Wed, May 22, 2019 at 10:15:56AM +0200, Jiri Slaby wrote:
> Look at the top of alloc_tty_struct: there is tty_ldisc_init. If
> tty_get_device fails here, you have to call tty_ldisc_deinit. Better,
> you should add a failure-handling tail to this function and "goto" there.
Thanks for your explaination, Jiri.
I will work on it.
Thanks
Gen

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] tty_io: Fix a missing-check bug in drivers/tty/tty_io.c
  2019-05-22 11:13         ` Gen Zhang
@ 2019-05-22 11:19           ` Johan Hovold
  2019-05-22 11:24             ` Gen Zhang
  0 siblings, 1 reply; 10+ messages in thread
From: Johan Hovold @ 2019-05-22 11:19 UTC (permalink / raw)
  To: Gen Zhang; +Cc: Johan Hovold, linux-kernel

On Wed, May 22, 2019 at 07:13:54PM +0800, Gen Zhang wrote:
> On Wed, May 22, 2019 at 12:29:00PM +0200, Johan Hovold wrote:
> > Where do you see that the kernel is dereferencing tty->dev without
> > checking for NULL first? If you can find that, then that would indeed be
> > a bug that needs fixing.
> Thanks for your reply, Johan!
> I examined the code but failed to find this situation.

Ok, so your claim in the commit message was incorrect:

	And tty->dev is dereferenced in the following codes.

> Anyway, checking return value of tty_get_device() is theoritically
> right. But tty->dev is never dereferenced, so checking is not needed.

No, sorry, it's not even theoretically correct. Our current code depends
on tty->dev sometimes being NULL. Your patch would specifically break
pseudo terminals.

> However, what if in later kernels tty->dev is dereferenced by some
> codes? Is it better to apply this check for this reason?

So for the above reason, no.

Johan

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] tty_io: Fix a missing-check bug in drivers/tty/tty_io.c
  2019-05-22 11:19           ` Johan Hovold
@ 2019-05-22 11:24             ` Gen Zhang
  0 siblings, 0 replies; 10+ messages in thread
From: Gen Zhang @ 2019-05-22 11:24 UTC (permalink / raw)
  To: Johan Hovold; +Cc: linux-kernel

On Wed, May 22, 2019 at 01:19:49PM +0200, Johan Hovold wrote:
> On Wed, May 22, 2019 at 07:13:54PM +0800, Gen Zhang wrote:
> > On Wed, May 22, 2019 at 12:29:00PM +0200, Johan Hovold wrote:
> > > Where do you see that the kernel is dereferencing tty->dev without
> > > checking for NULL first? If you can find that, then that would indeed be
> > > a bug that needs fixing.
> > Thanks for your reply, Johan!
> > I examined the code but failed to find this situation.
> 
> Ok, so your claim in the commit message was incorrect:
> 
> 	And tty->dev is dereferenced in the following codes.
> 
> > Anyway, checking return value of tty_get_device() is theoritically
> > right. But tty->dev is never dereferenced, so checking is not needed.
> 
> No, sorry, it's not even theoretically correct. Our current code depends
> on tty->dev sometimes being NULL. Your patch would specifically break
> pseudo terminals.
Thanks for your comments. I have to be very proficient in this module
to know this. Of course I am not.
Anyway, appreciate your replies, Johan.
Thanks
Gen
> 
> > However, what if in later kernels tty->dev is dereferenced by some
> > codes? Is it better to apply this check for this reason?
> 
> So for the above reason, no.
> 
> Johan

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-05-22 11:25 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-22  1:40 [PATCH] tty_io: Fix a missing-check bug in drivers/tty/tty_io.c Gen Zhang
2019-05-22  4:25 ` Jiri Slaby
2019-05-22  8:06   ` Gen Zhang
2019-05-22  8:15     ` Jiri Slaby
2019-05-22 10:29       ` Johan Hovold
2019-05-22 10:32         ` Jiri Slaby
2019-05-22 11:13         ` Gen Zhang
2019-05-22 11:19           ` Johan Hovold
2019-05-22 11:24             ` Gen Zhang
2019-05-22 11:18       ` Gen Zhang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).