* [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 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
* 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
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).