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 <[email protected]>
---
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;
}
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 <[email protected]>
>
> ---
> 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
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 <[email protected]>
> >
> > ---
> > 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
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 <[email protected]>
>>>
>>> ---
>>> 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
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 <[email protected]>
> >>>
> >>> ---
> >>> 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
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
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
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
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
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