[PATCH] fbcon: fix null-ptr-deref in fbcon_switch

Subsystems: framebuffer console, framebuffer core, framebuffer layer, the rest

STALE2321d

5 messages, 3 authors, 2020-03-29 · open the first message on its own page

[PATCH] fbcon: fix null-ptr-deref in fbcon_switch

From: Qiujun Huang <hidden>
Date: 2020-03-28 15:15:23 

Add check for vc_cons[logo_shown].d, as it can be released by
vt_ioctl(VT_DISALLOCATE).

Reported-by: syzbot+732528bae351682f1f27@syzkaller.appspotmail.com
Signed-off-by: Qiujun Huang <redacted>
---
 drivers/video/fbdev/core/fbcon.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index bb6ae995c2e5..7ee0f7b55829 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -2254,7 +2254,7 @@ static int fbcon_switch(struct vc_data *vc)
 		fbcon_update_softback(vc);
 	}
 
-	if (logo_shown >= 0) {
+	if (logo_shown >= 0 && vc_cons_allocated(logo_shown)) {
 		struct vc_data *conp2 = vc_cons[logo_shown].d;
 
 		if (conp2->vc_top = logo_lines
@@ -2852,7 +2852,7 @@ static void fbcon_scrolldelta(struct vc_data *vc, int lines)
 			return;
 		if (vc->vc_mode != KD_TEXT || !lines)
 			return;
-		if (logo_shown >= 0) {
+		if (logo_shown >= 0 && vc_cons_allocated(logo_shown)) {
 			struct vc_data *conp2 = vc_cons[logo_shown].d;
 
 			if (conp2->vc_top = logo_lines
-- 
2.17.1

Re: [PATCH] fbcon: fix null-ptr-deref in fbcon_switch

From: Daniel Vetter <hidden>
Date: 2020-03-28 16:31:49 

On Sat, Mar 28, 2020 at 4:15 PM Qiujun Huang [off-list ref] wrote:
Add check for vc_cons[logo_shown].d, as it can be released by
vt_ioctl(VT_DISALLOCATE).
Can you pls link to the syzbot report and distill the essence of the
crash/issue here in the commit message? As-is a bit unclear what's
going on. Patch itself looks correct.

Thanks, Daniel
quoted hunk
Reported-by: syzbot+732528bae351682f1f27@syzkaller.appspotmail.com
Signed-off-by: Qiujun Huang <redacted>
---
 drivers/video/fbdev/core/fbcon.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index bb6ae995c2e5..7ee0f7b55829 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -2254,7 +2254,7 @@ static int fbcon_switch(struct vc_data *vc)
                fbcon_update_softback(vc);
        }

-       if (logo_shown >= 0) {
+       if (logo_shown >= 0 && vc_cons_allocated(logo_shown)) {
                struct vc_data *conp2 = vc_cons[logo_shown].d;

                if (conp2->vc_top = logo_lines
@@ -2852,7 +2852,7 @@ static void fbcon_scrolldelta(struct vc_data *vc, int lines)
                        return;
                if (vc->vc_mode != KD_TEXT || !lines)
                        return;
-               if (logo_shown >= 0) {
+               if (logo_shown >= 0 && vc_cons_allocated(logo_shown)) {
                        struct vc_data *conp2 = vc_cons[logo_shown].d;

                        if (conp2->vc_top = logo_lines
--
2.17.1

--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] fbcon: fix null-ptr-deref in fbcon_switch

From: Qiujun Huang <hidden>
Date: 2020-03-28 16:57:44 

On Sun, Mar 29, 2020 at 12:31 AM Daniel Vetter [off-list ref] wrote:
On Sat, Mar 28, 2020 at 4:15 PM Qiujun Huang [off-list ref] wrote:
quoted
Add check for vc_cons[logo_shown].d, as it can be released by
vt_ioctl(VT_DISALLOCATE).
Can you pls link to the syzbot report and distill the essence of the
crash/issue here in the commit message? As-is a bit unclear what's
going on. Patch itself looks correct.
https://lkml.org/lkml/2020/3/27/403
Thanks.

Thanks, Daniel
quoted
Reported-by: syzbot+732528bae351682f1f27@syzkaller.appspotmail.com
Signed-off-by: Qiujun Huang <redacted>
---
 drivers/video/fbdev/core/fbcon.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index bb6ae995c2e5..7ee0f7b55829 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -2254,7 +2254,7 @@ static int fbcon_switch(struct vc_data *vc)
                fbcon_update_softback(vc);
        }

-       if (logo_shown >= 0) {
+       if (logo_shown >= 0 && vc_cons_allocated(logo_shown)) {
                struct vc_data *conp2 = vc_cons[logo_shown].d;

                if (conp2->vc_top = logo_lines
@@ -2852,7 +2852,7 @@ static void fbcon_scrolldelta(struct vc_data *vc, int lines)
                        return;
                if (vc->vc_mode != KD_TEXT || !lines)
                        return;
-               if (logo_shown >= 0) {
+               if (logo_shown >= 0 && vc_cons_allocated(logo_shown)) {
                        struct vc_data *conp2 = vc_cons[logo_shown].d;

                        if (conp2->vc_top = logo_lines
--
2.17.1

--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] fbcon: fix null-ptr-deref in fbcon_switch

From: Sam Ravnborg <hidden>
Date: 2020-03-28 18:13:07 

Hi Qiujun

Thanks for looking into the sysbot bugs.

On Sat, Mar 28, 2020 at 11:15:10PM +0800, Qiujun Huang wrote:
quoted hunk
Add check for vc_cons[logo_shown].d, as it can be released by
vt_ioctl(VT_DISALLOCATE).

Reported-by: syzbot+732528bae351682f1f27@syzkaller.appspotmail.com
Signed-off-by: Qiujun Huang <redacted>
---
 drivers/video/fbdev/core/fbcon.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index bb6ae995c2e5..7ee0f7b55829 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -2254,7 +2254,7 @@ static int fbcon_switch(struct vc_data *vc)
 		fbcon_update_softback(vc);
 	}
 
-	if (logo_shown >= 0) {
+	if (logo_shown >= 0 && vc_cons_allocated(logo_shown)) {
 		struct vc_data *conp2 = vc_cons[logo_shown].d;
 
 		if (conp2->vc_top = logo_lines
@@ -2852,7 +2852,7 @@ static void fbcon_scrolldelta(struct vc_data *vc, int lines)
 			return;
 		if (vc->vc_mode != KD_TEXT || !lines)
 			return;
-		if (logo_shown >= 0) {
+		if (logo_shown >= 0 && vc_cons_allocated(logo_shown)) {
 			struct vc_data *conp2 = vc_cons[logo_shown].d;
 
 			if (conp2->vc_top = logo_lines
I am not familiar with this code.

But it looks like you try to avoid the sympton
which is that logo_shown has a wrong value after a
vc is deallocated, and do not fix the root cause.

We have:

vt_ioctl(VT_DISALLOCATE)
 |
 +- vc_deallocate()
     |
     +- visual_deinit()
         |
	 +- vc->vc_sw->con_deinit(vc)
	     |
	     +- fbcon_deinit()

Would it be better to update logo_shown
in fbcon_deinit()?
Then we will not try to do anything with
the logo in fbcon_switch().

fbcon_deinit() is called with console locked
so there should not be any races.

I did not stare long enough on the code to come up with a patch,
but this may be a better way to fix it.

	Sam

Re: [PATCH] fbcon: fix null-ptr-deref in fbcon_switch

From: Qiujun Huang <hidden>
Date: 2020-03-29 01:04:56 

On Sun, Mar 29, 2020 at 2:13 AM Sam Ravnborg [off-list ref] wrote:
Hi Qiujun

Thanks for looking into the sysbot bugs.

On Sat, Mar 28, 2020 at 11:15:10PM +0800, Qiujun Huang wrote:
quoted
Add check for vc_cons[logo_shown].d, as it can be released by
vt_ioctl(VT_DISALLOCATE).

Reported-by: syzbot+732528bae351682f1f27@syzkaller.appspotmail.com
Signed-off-by: Qiujun Huang <redacted>
---
 drivers/video/fbdev/core/fbcon.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index bb6ae995c2e5..7ee0f7b55829 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -2254,7 +2254,7 @@ static int fbcon_switch(struct vc_data *vc)
              fbcon_update_softback(vc);
      }

-     if (logo_shown >= 0) {
+     if (logo_shown >= 0 && vc_cons_allocated(logo_shown)) {
              struct vc_data *conp2 = vc_cons[logo_shown].d;

              if (conp2->vc_top = logo_lines
@@ -2852,7 +2852,7 @@ static void fbcon_scrolldelta(struct vc_data *vc, int lines)
                      return;
              if (vc->vc_mode != KD_TEXT || !lines)
                      return;
-             if (logo_shown >= 0) {
+             if (logo_shown >= 0 && vc_cons_allocated(logo_shown)) {
                      struct vc_data *conp2 = vc_cons[logo_shown].d;

                      if (conp2->vc_top = logo_lines
I am not familiar with this code.

But it looks like you try to avoid the sympton
which is that logo_shown has a wrong value after a
vc is deallocated, and do not fix the root cause.

We have:

vt_ioctl(VT_DISALLOCATE)
 |
 +- vc_deallocate()
     |
     +- visual_deinit()
         |
         +- vc->vc_sw->con_deinit(vc)
             |
             +- fbcon_deinit()

Would it be better to update logo_shown
in fbcon_deinit()?
Then we will not try to do anything with
the logo in fbcon_switch().

fbcon_deinit() is called with console locked
so there should not be any races.
Get that, thanks.

I did not stare long enough on the code to come up with a patch,
but this may be a better way to fix it.

        Sam
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help