omapfb_mmap() has a race condition with OMAPFB_SETUP_PLANE ioctl that
can lead to use-after-free:

The fb_mmap() entry point holds mm_lock but not lock (fb_info->lock),
while ioctl handlers like OMAPFB_SETUP_PLANE hold lock but not mm_lock.
This allows concurrent execution.

In omapfb_mmap():
1. rg = omapfb_get_mem_region(ofbi->region);      // Get old region ref
2. start = omapfb_get_region_paddr(ofbi);          // Read from NEW region
3. len = fix->smem_len;                             // Read from NEW region
4. vm_iomap_memory(vma, start, len);               // Map NEW region memory
5. atomic_inc(&rg->map_count);                      // Increment OLD region!

Concurrently, OMAPFB_SETUP_PLANE can:
- Reassign ofbi->region = new_rg
- Update fix->smem_len
- OMAPFB_SETUP_MEM then checks NEW region's map_count (0!) and frees it

This leaves userspace with a mapping to freed physical memory.

The fix is to read all required values (start, len) from the same
region reference (rg) that will have its map_count incremented,
preventing the region from being freed while still mapped.

Cc: stable@vger.kernel.org
Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>

---
 Change in V2:
  -Restore fix->smem_len to maintain VRFB sparse mapping.
  -Increment map_count before mapping to prevent use-after-free
   on driver unload
  -Add proper error handling for map_count
---
 drivers/video/fbdev/omap2/omapfb/omapfb-main.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/omap2/omapfb/omapfb-main.c b/drivers/video/fbdev/omap2/omapfb/omapfb-main.c
index d70deb6a9150..046892682fc6 100644
--- a/drivers/video/fbdev/omap2/omapfb/omapfb-main.c
+++ b/drivers/video/fbdev/omap2/omapfb/omapfb-main.c
@@ -1099,7 +1099,11 @@ static int omapfb_mmap(struct fb_info *fbi, struct vm_area_struct *vma)
 
 	rg = omapfb_get_mem_region(ofbi->region);
 
-	start = omapfb_get_region_paddr(ofbi);
+	if (ofbi->rotation_type == OMAP_DSS_ROT_VRFB)
+		start = rg->vrfb.paddr[0];
+	else
+		start = rg->paddr;
+
 	len = fix->smem_len;
 
 	DBG("user mmap region start %lx, len %d, off %lx\n", start, len,
@@ -1109,6 +1113,8 @@ static int omapfb_mmap(struct fb_info *fbi, struct vm_area_struct *vma)
 	vma->vm_ops = &mmap_user_ops;
 	vma->vm_private_data = rg;
 
+	atomic_inc(&rg->map_count);
+
 	r = vm_iomap_memory(vma, start, len);
 	if (r)
 		goto error;
@@ -1121,6 +1127,7 @@ static int omapfb_mmap(struct fb_info *fbi, struct vm_area_struct *vma)
 	return 0;
 
 error:
+	atomic_dec(&rg->map_count);
 	omapfb_put_mem_region(rg);
 
 	return r;
-- 
2.25.1

On 6/2/26 10:54, Hongling Zeng wrote:
> omapfb_mmap() has a race condition with OMAPFB_SETUP_PLANE ioctl that
> can lead to use-after-free:
> 
> The fb_mmap() entry point holds mm_lock but not lock (fb_info->lock),
> while ioctl handlers like OMAPFB_SETUP_PLANE hold lock but not mm_lock.
> This allows concurrent execution.
> 
> In omapfb_mmap():
> 1. rg = omapfb_get_mem_region(ofbi->region);      // Get old region ref
> 2. start = omapfb_get_region_paddr(ofbi);          // Read from NEW region
> 3. len = fix->smem_len;                             // Read from NEW region
> 4. vm_iomap_memory(vma, start, len);               // Map NEW region memory
> 5. atomic_inc(&rg->map_count);                      // Increment OLD region!
> 
> Concurrently, OMAPFB_SETUP_PLANE can:
> - Reassign ofbi->region = new_rg
> - Update fix->smem_len
> - OMAPFB_SETUP_MEM then checks NEW region's map_count (0!) and frees it
> 
> This leaves userspace with a mapping to freed physical memory.
> 
> The fix is to read all required values (start, len) from the same
> region reference (rg) that will have its map_count incremented,
> preventing the region from being freed while still mapped.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
> 
> ---
>   Change in V2:
>    -Restore fix->smem_len to maintain VRFB sparse mapping.
>    -Increment map_count before mapping to prevent use-after-free
>     on driver unload
>    -Add proper error handling for map_count
> ---
>   drivers/video/fbdev/omap2/omapfb/omapfb-main.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/video/fbdev/omap2/omapfb/omapfb-main.c b/drivers/video/fbdev/omap2/omapfb/omapfb-main.c
> index d70deb6a9150..046892682fc6 100644
> --- a/drivers/video/fbdev/omap2/omapfb/omapfb-main.c
> +++ b/drivers/video/fbdev/omap2/omapfb/omapfb-main.c
> @@ -1099,7 +1099,11 @@ static int omapfb_mmap(struct fb_info *fbi, struct vm_area_struct *vma)
>   
>   	rg = omapfb_get_mem_region(ofbi->region);
>   
> -	start = omapfb_get_region_paddr(ofbi);
> +	if (ofbi->rotation_type == OMAP_DSS_ROT_VRFB)
> +		start = rg->vrfb.paddr[0];
> +	else
> +		start = rg->paddr;
> +
>   	len = fix->smem_len;
>   
>   	DBG("user mmap region start %lx, len %d, off %lx\n", start, len,
> @@ -1109,6 +1113,8 @@ static int omapfb_mmap(struct fb_info *fbi, struct vm_area_struct *vma)
>   	vma->vm_ops = &mmap_user_ops;
>   	vma->vm_private_data = rg;
>   
> +	atomic_inc(&rg->map_count);
> +
>   	r = vm_iomap_memory(vma, start, len);
>   	if (r)
>   		goto error;
In the code there is another atomic_inc() here....
With your patch, map_count is incremented twice.
Is this right?

Helge