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
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)
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
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
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
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
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
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,
dri-devel@lists.freedesktop.org
-
'Max Staudt -
Daniel Vetter -
Max Staudt