[patch 1/1] fb: fix potential deadlock between lock_fb_info and console_lock

From: Andrea Righi <redacted>
Subject: fb: fix potential deadlock between lock_fb_info and console_lock

fb_set_suspend() must be called with the console semaphore held, which
means the code path coming in here will first take the console_lock() and
then call lock_fb_info().

However several framebuffer ioctl commands acquire these locks in reverse
order (lock_fb_info() and then console_lock()).  This gives rise to
potential AB-BA deadlock.

Fix this by changing the order of acquisition in the ioctl commands that
make use of console_lock().

Signed-off-by: Andrea Righi <redacted>
Reported-by: Peter Nordström (Palm GBU) <redacted>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: <stable@kernel.org>

Signed-off-by: Andrew Morton <redacted>
---

 drivers/video/fbmem.c |   24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff -puN drivers/video/fbmem.c~fb-fix-potential-deadlock-between-lock_fb_info-and-console_lock drivers/video/fbmem.c

--- a/drivers/video/fbmem.c~fb-fix-potential-deadlock-between-lock_fb_info-and-console_lock
+++ a/drivers/video/fbmem.c

@@ -1076,14 +1076,16 @@ static long do_fb_ioctl(struct fb_info *
 	case FBIOPUT_VSCREENINFO:
 		if (copy_from_user(&var, argp, sizeof(var)))
 			return -EFAULT;
-		if (!lock_fb_info(info))
-			return -ENODEV;
 		console_lock();
+		if (!lock_fb_info(info)) {
+			console_unlock();
+			return -ENODEV;
+		}
 		info->flags |= FBINFO_MISC_USEREVENT;
 		ret = fb_set_var(info, &var);
 		info->flags &= ~FBINFO_MISC_USEREVENT;
-		console_unlock();
 		unlock_fb_info(info);
+		console_unlock();
 		if (!ret && copy_to_user(argp, &var, sizeof(var)))
 			ret = -EFAULT;
 		break;

@@ -1112,12 +1114,14 @@ static long do_fb_ioctl(struct fb_info *
 	case FBIOPAN_DISPLAY:
 		if (copy_from_user(&var, argp, sizeof(var)))
 			return -EFAULT;
-		if (!lock_fb_info(info))
-			return -ENODEV;
 		console_lock();
+		if (!lock_fb_info(info)) {
+			console_unlock();
+			return -ENODEV;
+		}
 		ret = fb_pan_display(info, &var);
-		console_unlock();
 		unlock_fb_info(info);
+		console_unlock();
 		if (ret = 0 && copy_to_user(argp, &var, sizeof(var)))
 			return -EFAULT;
 		break;

@@ -1159,14 +1163,16 @@ static long do_fb_ioctl(struct fb_info *
 		unlock_fb_info(info);
 		break;
 	case FBIOBLANK:
-		if (!lock_fb_info(info))
-			return -ENODEV;
 		console_lock();
+		if (!lock_fb_info(info)) {
+			console_unlock();
+			return -ENODEV;
+		}
 		info->flags |= FBINFO_MISC_USEREVENT;
 		ret = fb_blank(info, arg);
 		info->flags &= ~FBINFO_MISC_USEREVENT;
-		console_unlock();
 		unlock_fb_info(info);
+		console_unlock();
 		break;
 	default:
 		if (!lock_fb_info(info))

_

Hi Andrew,

On 09/20/2011 09:11 PM, akpm@google.com wrote:

From: Andrea Righi <redacted>
Subject: fb: fix potential deadlock between lock_fb_info and console_lock

fb_set_suspend() must be called with the console semaphore held, which
means the code path coming in here will first take the console_lock() and
then call lock_fb_info().

However several framebuffer ioctl commands acquire these locks in reverse
order (lock_fb_info() and then console_lock()).  This gives rise to
potential AB-BA deadlock.

Fix this by changing the order of acquisition in the ioctl commands that
make use of console_lock().

I already have another patch [1] that fixes the same issue in a different way.
It looks less risky than yours and got more feedback.


Best regards,

Florian Tobias Schandinat


[1] http://marc.info/?l=linux-kernel&m0833638508657&w=2

On Tue, Sep 20, 2011 at 09:46:08PM +0000, Florian Tobias Schandinat wrote:

Hi Andrew,

On 09/20/2011 09:11 PM, akpm@google.com wrote:

quoted

From: Andrea Righi <redacted>
Subject: fb: fix potential deadlock between lock_fb_info and console_lock

fb_set_suspend() must be called with the console semaphore held, which
means the code path coming in here will first take the console_lock() and
then call lock_fb_info().

However several framebuffer ioctl commands acquire these locks in reverse
order (lock_fb_info() and then console_lock()).  This gives rise to
potential AB-BA deadlock.

Fix this by changing the order of acquisition in the ioctl commands that
make use of console_lock().

I already have another patch [1] that fixes the same issue in a different way.
It looks less risky than yours and got more feedback.


Best regards,

Florian Tobias Schandinat


[1] http://marc.info/?l=linux-kernel&m0833638508657&w=2

Looks better than my version.

For what it's worth it:

Reviewed-by: Andrea Righi <redacted>

Thanks,
-Andrea