[PATCH 0/3] Fix kernel WARNING in TTM/Bochs KMS driver

[PATCH 0/3] Fix kernel WARNING in TTM/Bochs KMS driver

['Max Staudt]

This patch set addresses an instance of dangling/use-after-free
pointers that can occur in the bochsdrm driver.

The original report can be found at:
https://lkml.kernel.org/g/s5hy49ue4y0.wl-tiwai@suse.de

It can be tested using 'qemu-system-x86_64 -vga std' with any kernel
recent enough to ship the bochsdrm driver, and with the fbdev
emulation active.
In other words: Any modern distro's default setup will do.


Max Staudt



 [PATCH 1/3] Add missing "goto out" after fbops->fb_open()
 [PATCH 2/3] Define fb_open_adj_file()
 [PATCH 3/3] Add fb_open_adj_file() to bochsdrmfb to avoid

[PATCH 1/3] Add missing "goto out" after fbops->fb_open()

['Max Staudt]

From: Max Staudt <mstaudt@suse.de>

It doesn't make sense to execute any further actions after a failed
call to fbops->fb_open().

Signed-off-by: Max Staudt <mstaudt@suse.de>
---
 drivers/video/fbdev/core/fbmem.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 76c1ad9..913c496 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1462,8 +1462,10 @@ __releases(&info->lock)
 	file->private_data = info;
 	if (info->fbops->fb_open) {
 		res = info->fbops->fb_open(info,1);
-		if (res)
+		if (res) {
 			module_put(info->fbops->owner);
+			goto out;
+		}
 	}
 #ifdef CONFIG_FB_DEFERRED_IO
 	if (info->fbdefio)
-- 
2.6.6

[PATCH 2/3] Define fb_open_adj_file()

['Max Staudt]

From: Max Staudt <mstaudt@suse.de>

This callback from fb_open() allows a fbdev driver to adjust things such
as file->f_mapping to better represent the internal structures.

This is needed to allow TTM drivers using ttm_fbdev_mmap() to properly
set file->f_mapping to TTM's address_space from bo->bdev->dev_mapping,
as is done form /dev/drm/card* in drm_open().

Currently, bochsdrmfb is the only driver requiring this, and this
patch is a prerequisite to implement this in bochsdrmfb.

Signed-off-by: Max Staudt <mstaudt@suse.de>
---
 drivers/video/fbdev/core/fbmem.c | 13 +++++++++++++
 include/linux/fb.h               |  7 +++++++
 2 files changed, 20 insertions(+)

diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 913c496..ff5000e 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1471,6 +1471,19 @@ __releases(&info->lock)
 	if (info->fbdefio)
 		fb_deferred_io_open(info, inode, file);
 #endif
+	if (info->fbops->fb_open_adj_file) {
+		res = info->fbops->fb_open_adj_file(info, file);
+		if (res) {
+			/* If we got here, we also want to undo anything
+			 * that fbops->fb_open() may have done.
+			 */
+			if (info->fbops->fb_release)
+				info->fbops->fb_release(info,1);
+
+			module_put(info->fbops->owner);
+			goto out;
+		}
+	}
 out:
 	mutex_unlock(&info->lock);
 	if (res)
diff --git a/include/linux/fb.h b/include/linux/fb.h
index dfe8835..4131496 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -320,6 +320,13 @@ struct fb_ops {
 	/* called at KDB enter and leave time to prepare the console */
 	int (*fb_debug_enter)(struct fb_info *info);
 	int (*fb_debug_leave)(struct fb_info *info);
+
+	/* Called after fb_open() to adjust mappings when using
+	 * complex backends such as TTM.
+	 * For example, the bochsdrmfb driver needs to adjust
+	 * file->f_mapping so it can use ttm_fbdev_mmap().
+	 */
+	int (*fb_open_adj_file)(struct fb_info *info, struct file *file);
 };
 
 #ifdef CONFIG_FB_TILEBLITTING
-- 
2.6.6

Re: [PATCH 2/3] Define fb_open_adj_file()

[Daniel Vetter]

On Mon, May 23, 2016 at 12:48:52PM +0200, 'Max Staudt wrote:
> From: Max Staudt <mstaudt@suse.de>
> 
> This callback from fb_open() allows a fbdev driver to adjust things such
> as file->f_mapping to better represent the internal structures.
> 
> This is needed to allow TTM drivers using ttm_fbdev_mmap() to properly
> set file->f_mapping to TTM's address_space from bo->bdev->dev_mapping,
> as is done form /dev/drm/card* in drm_open().
> 
> Currently, bochsdrmfb is the only driver requiring this, and this
> patch is a prerequisite to implement this in bochsdrmfb.
> 
> Signed-off-by: Max Staudt <mstaudt@suse.de>

Do we _really_ care about fbdev mmap support so much that we want to add
more hacks all over the place (in each driver) to make it work? Given that
fbdev is officially in the "no more drivers" territory, I'm not sure we
want this either.

The trouble is that fbdev mmap and kms dumb buffer mmap have different
semantics and so every driver needs to implement both if they're special
in any kind of way.

If we can't say no to fbdev mmap then I think we should implement
something that works for all drivers. I'm thinking of allocating just a
pile of pages in the fbdev emulation, and then copying them over using the
defio stuff we just merged for 4.7 into a dumb bo, plus calling dirtyfb.
Or at least something along those lines. Of couse just opt-in, in case the
driver can do a more traditional mmio fbdev mmapping.
-Daniel

> ---
>  drivers/video/fbdev/core/fbmem.c | 13 +++++++++++++
>  include/linux/fb.h               |  7 +++++++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index 913c496..ff5000e 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1471,6 +1471,19 @@ __releases(&info->lock)
>  	if (info->fbdefio)
>  		fb_deferred_io_open(info, inode, file);
>  #endif
> +	if (info->fbops->fb_open_adj_file) {
> +		res = info->fbops->fb_open_adj_file(info, file);
> +		if (res) {
> +			/* If we got here, we also want to undo anything
> +			 * that fbops->fb_open() may have done.
> +			 */
> +			if (info->fbops->fb_release)
> +				info->fbops->fb_release(info,1);
> +
> +			module_put(info->fbops->owner);
> +			goto out;
> +		}
> +	}
>  out:
>  	mutex_unlock(&info->lock);
>  	if (res)
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index dfe8835..4131496 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -320,6 +320,13 @@ struct fb_ops {
>  	/* called at KDB enter and leave time to prepare the console */
>  	int (*fb_debug_enter)(struct fb_info *info);
>  	int (*fb_debug_leave)(struct fb_info *info);
> +
> +	/* Called after fb_open() to adjust mappings when using
> +	 * complex backends such as TTM.
> +	 * For example, the bochsdrmfb driver needs to adjust
> +	 * file->f_mapping so it can use ttm_fbdev_mmap().
> +	 */
> +	int (*fb_open_adj_file)(struct fb_info *info, struct file *file);
>  };
>  
>  #ifdef CONFIG_FB_TILEBLITTING
> -- 
> 2.6.6
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 2/3] Define fb_open_adj_file()

[Max Staudt]

Hi Daniel,

Thanks for the feedback! Comments below:


On 05/23/2016 03:44 PM, Daniel Vetter wrote:
> Do we _really_ care about fbdev mmap support so much that we want to add
> more hacks all over the place (in each driver) to make it work? Given that
> fbdev is officially in the "no more drivers" territory, I'm not sure we
> want this either.
> 
> The trouble is that fbdev mmap and kms dumb buffer mmap have different
> semantics and so every driver needs to implement both if they're special
> in any kind of way.

DRM drivers already have fbdev hacks in place in order to implement both semantics.

With this change, only bochsdrm(fb) will need to be touched, and if no further DRM driver implements fbdev support the way bochsdrmfb does, then no further driver will need this code.


This patch set is really just about setting file->f_mapping in bochsdrmfb's fb_open(), so we finally get rid of the WARNING.

We could also pass file as a third paramenter in fb_open(), changing the function definition in all fbdev drivers. I'm happy to change the patch in that way if this is preferred.


> If we can't say no to fbdev mmap then I think we should implement
> something that works for all drivers. I'm thinking of allocating just a
> pile of pages in the fbdev emulation, and then copying them over using the
> defio stuff we just merged for 4.7 into a dumb bo, plus calling dirtyfb.
> Or at least something along those lines. Of couse just opt-in, in case the
> driver can do a more traditional mmio fbdev mmapping.

Well, the mmap code is standard fbdev and it's already there, so why not keep it?

All this patch series does is allowing bochsdrmfb to work properly until fbdev support is finally removed from the kernel.



Max

Re: [PATCH 2/3] Define fb_open_adj_file()

[Daniel Vetter]

On Tue, May 24, 2016 at 6:28 PM, Max Staudt <mstaudt@suse.de> wrote:
> Hi Daniel,
>
> Thanks for the feedback! Comments below:
>
>
> On 05/23/2016 03:44 PM, Daniel Vetter wrote:
>> Do we _really_ care about fbdev mmap support so much that we want to add
>> more hacks all over the place (in each driver) to make it work? Given that
>> fbdev is officially in the "no more drivers" territory, I'm not sure we
>> want this either.
>>
>> The trouble is that fbdev mmap and kms dumb buffer mmap have different
>> semantics and so every driver needs to implement both if they're special
>> in any kind of way.
>
> DRM drivers already have fbdev hacks in place in order to implement both semantics.

A lot of these fbdev hacks just disappeared since we've gained generic
defio support in 4.7. Is your assertion still true on 4.7? I really
don't want to grow hacks all over the place for something we should
try to solve once for everyone.

> With this change, only bochsdrm(fb) will need to be touched, and if no further DRM driver implements fbdev support the way bochsdrmfb does, then no further driver will need this code.

See above - we've grown meanwhile enough drm drivers which need
defio/special mmap support that there's now a pile of generic code and
refactoring in 4.7. It's not just bochsdrm by far. Also pretty much
every drm driver has their own pile of fbdev mmap hacks.

> This patch set is really just about setting file->f_mapping in bochsdrmfb's fb_open(), so we finally get rid of the WARNING.
>
> We could also pass file as a third paramenter in fb_open(), changing the function definition in all fbdev drivers. I'm happy to change the patch in that way if this is preferred.
>
>
>> If we can't say no to fbdev mmap then I think we should implement
>> something that works for all drivers. I'm thinking of allocating just a
>> pile of pages in the fbdev emulation, and then copying them over using the
>> defio stuff we just merged for 4.7 into a dumb bo, plus calling dirtyfb.
>> Or at least something along those lines. Of couse just opt-in, in case the
>> driver can do a more traditional mmio fbdev mmapping.
>
> Well, the mmap code is standard fbdev and it's already there, so why not keep it?
>
> All this patch series does is allowing bochsdrmfb to work properly until fbdev support is finally removed from the kernel.

I don't think we can remove fbdev in the next 10 years unfortunately.
It's deprecated insofar that no new drivers are accepted. Instead
proper kms drivers (with fbdev emulation) should be created.

But that means another decade of new drivers in kms that need to add
hacks for fbdev mmap support, if we don't start to make this simpler
and avoid the need for such special cases.

Anyway, that's kinda the high-level reasons why I jumped on this. I
think for the next steps in the discussion you need to take a look at
what's new in 4.7. And then we need to figure out what we need to add
to improve fbdev mmap further.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/3] Define fb_open_adj_file()

[Max Staudt]

On 05/24/2016 06:51 PM, Daniel Vetter wrote:
> On Tue, May 24, 2016 at 6:28 PM, Max Staudt <mstaudt@suse.de> wrote:
>> Hi Daniel,
>>
>> Thanks for the feedback! Comments below:
>>
>>
>> On 05/23/2016 03:44 PM, Daniel Vetter wrote:
>>> Do we _really_ care about fbdev mmap support so much that we want to add
>>> more hacks all over the place (in each driver) to make it work? Given that
>>> fbdev is officially in the "no more drivers" territory, I'm not sure we
>>> want this either.
>>>
>>> The trouble is that fbdev mmap and kms dumb buffer mmap have different
>>> semantics and so every driver needs to implement both if they're special
>>> in any kind of way.
>>
>> DRM drivers already have fbdev hacks in place in order to implement both semantics.
> 
> A lot of these fbdev hacks just disappeared since we've gained generic
> defio support in 4.7. Is your assertion still true on 4.7? I really
> don't want to grow hacks all over the place for something we should
> try to solve once for everyone.
> 
>> With this change, only bochsdrm(fb) will need to be touched, and if no further DRM driver implements fbdev support the way bochsdrmfb does, then no further driver will need this code.
> 
> See above - we've grown meanwhile enough drm drivers which need
> defio/special mmap support that there's now a pile of generic code and
> refactoring in 4.7. It's not just bochsdrm by far. Also pretty much
> every drm driver has their own pile of fbdev mmap hacks.
> 
>> This patch set is really just about setting file->f_mapping in bochsdrmfb's fb_open(), so we finally get rid of the WARNING.
>>
>> We could also pass file as a third paramenter in fb_open(), changing the function definition in all fbdev drivers. I'm happy to change the patch in that way if this is preferred.
>>
>>
>>> If we can't say no to fbdev mmap then I think we should implement
>>> something that works for all drivers. I'm thinking of allocating just a
>>> pile of pages in the fbdev emulation, and then copying them over using the
>>> defio stuff we just merged for 4.7 into a dumb bo, plus calling dirtyfb.
>>> Or at least something along those lines. Of couse just opt-in, in case the
>>> driver can do a more traditional mmio fbdev mmapping.
>>
>> Well, the mmap code is standard fbdev and it's already there, so why not keep it?
>>
>> All this patch series does is allowing bochsdrmfb to work properly until fbdev support is finally removed from the kernel.
> 
> I don't think we can remove fbdev in the next 10 years unfortunately.
> It's deprecated insofar that no new drivers are accepted. Instead
> proper kms drivers (with fbdev emulation) should be created.
> 
> But that means another decade of new drivers in kms that need to add
> hacks for fbdev mmap support, if we don't start to make this simpler
> and avoid the need for such special cases.
> 
> Anyway, that's kinda the high-level reasons why I jumped on this. I
> think for the next steps in the discussion you need to take a look at
> what's new in 4.7. And then we need to figure out what we need to add
> to improve fbdev mmap further.
> -Daniel


Thanks, I'll look into it.

Hm, sounds like it's best to ask the maintainer for advice, too.


Gerd, could you please have a look at this thread?



Thanks

Max

[PATCH 3/3] Add fb_open_adj_file() to bochsdrmfb to avoid use-after-free

['Max Staudt]

From: Max Staudt <mstaudt@suse.de>

Currently, when using bochsdrm(fb), opening /dev/drm/card0 will adjust
file->f_mapping, but opening /dev/fb0 will not.
This may result in dangling pointers from user space when memory is
evicted, and therefore in use-after-free crashes when using the emulated
framebuffer device.

Bochs is the only driver to use the TTM fbdev integration
(ttm_fbdev_mmap()), and thus the only one able to trigger this case.
It is highlighted by the WARN_ON() in ttm_bo_vm_open() in
drivers/gpu/drm/ttm/ttm_bo_vm.c which is printed upon splitting
a VMA (see reproducer code below):

WARN_ON(bo->bdev->dev_mapping != vma->vm_file->f_mapping);

This happens because ttm_fbdev_mmap() sets the VMA's vm_ops pointer
to point to the TTM memory handling functions, without the file's
f_mapping having been adjusted. This means that file->f_mapping would
still point to the address_space for the inode in the file system.

This patch avoids dangling/use-after-free pointers that remain after
TTM decides to evict the memory referenced by bo->bdev->dev_mapping,
as the VMAs of a mmap()ed /dev/fb0 belong to /dev/fb0 instead of the
anonymous inode dev->anon_inode used by DRM.
It basically mimics the adjustment of file->f_mapping that is also
performed in drm_open() in drivers/gpu/drm/drm_fops.c.

For the original report, see:
https://lkml.kernel.org/g/s5hy49ue4y0.wl-tiwai@suse.de

The reproducer code is as follows:

----

int main(void)
{
        void *addr;

        int fd = open("/dev/fb0", O_RDONLY);
        if (fd < 0)
                err(1, "open");

        addr = mmap(0, 8192, PROT_READ, MAP_SHARED, fd, 0);
        if (addr == MAP_FAILED)
                err(1, "1st mmap");

        if (mmap(addr, 4096, PROT_READ, MAP_SHARED | MAP_FIXED | MAP_ANONYMOUS, -1, 0) == MAP_FAILED)
                err(1, "2nd mmap");

        return 0;
}

----

Signed-off-by: Max Staudt <mstaudt@suse.de>
---
 drivers/gpu/drm/bochs/bochs.h       |  1 +
 drivers/gpu/drm/bochs/bochs_fbdev.c | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/bochs/bochs.h b/drivers/gpu/drm/bochs/bochs.h
index 19b5ada..c37d30a 100644
--- a/drivers/gpu/drm/bochs/bochs.h
+++ b/drivers/gpu/drm/bochs/bochs.h
@@ -1,5 +1,6 @@
 #include <linux/io.h>
 #include <linux/fb.h>
+#include <linux/fs.h>
 #include <linux/console.h>
 
 #include <drm/drmP.h>
diff --git a/drivers/gpu/drm/bochs/bochs_fbdev.c b/drivers/gpu/drm/bochs/bochs_fbdev.c
index 7520bf8..fd70449 100644
--- a/drivers/gpu/drm/bochs/bochs_fbdev.c
+++ b/drivers/gpu/drm/bochs/bochs_fbdev.c
@@ -20,6 +20,24 @@ static int bochsfb_mmap(struct fb_info *info,
 	return ttm_fbdev_mmap(vma, &bo->bo);
 }
 
+static int bochsfb_open_adj_file(struct fb_info *info, struct file *file)
+{
+	struct drm_fb_helper *helper = (struct drm_fb_helper *)info->par;
+	struct bochs_device *bochs =
+		container_of(helper, struct bochs_device, fb.helper);
+	struct drm_device *dev = bochs->dev;
+
+	/* Correct f_mapping here so it's consistent across the
+	 * fd's lifetime.
+	 * If this isn't done, the WARN_ON() in ttm_bo_vm_open()
+	 * will trip upon vma_split(), hinting that other memory
+	 * management nastiness may occur when TTM evicts.
+	 */
+	file->f_mapping = dev->anon_inode->i_mapping;
+
+	return 0;
+}
+
 static struct fb_ops bochsfb_ops = {
 	.owner = THIS_MODULE,
 	.fb_check_var = drm_fb_helper_check_var,
@@ -31,6 +49,7 @@ static struct fb_ops bochsfb_ops = {
 	.fb_blank = drm_fb_helper_blank,
 	.fb_setcmap = drm_fb_helper_setcmap,
 	.fb_mmap = bochsfb_mmap,
+	.fb_open_adj_file = bochsfb_open_adj_file,
 };
 
 static int bochsfb_create_object(struct bochs_device *bochs,
-- 
2.6.6