On Wed, Dec 4, 2024 at 10:09 AM Kasireddy, Vivek vivek.kasireddy@intel.com wrote:
Subject: [PATCH 1/3] udmabuf: fix racy memfd sealing check
The current check_memfd_seals() is racy: Since we first do check_memfd_seals() and then udmabuf_pin_folios() without holding any relevant lock across both, F_SEAL_WRITE can be set in between. This is problematic because we can end up holding pins to pages in a write-sealed memfd.
Fix it using the inode lock, that's probably the easiest way. In the future, we might want to consider moving this logic into memfd, especially if anyone else wants to use memfd_pin_folios().
Reported-by: Julian Orth ju.orth@gmail.com Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219106 Closes: https://lore.kernel.org/r/CAG48ez0w8HrFEZtJkfmkVKFDhE5aP7nz=obrimeTg pD+StkV9w@mail.gmail.com Fixes: fbb0de795078 ("Add udmabuf misc device") Cc: stable@vger.kernel.org Signed-off-by: Jann Horn jannh@google.com
drivers/dma-buf/udmabuf.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c index 8ce1f074c2d32a0a9f59ff7184359e37d56548c6..662b9a26e06668bf59ab36d0 7c0648c7b02ee5ae 100644 --- a/drivers/dma-buf/udmabuf.c +++ b/drivers/dma-buf/udmabuf.c @@ -436,14 +436,15 @@ static long udmabuf_create(struct miscdevice *device, goto err; }
inode_lock_shared(memfd->f_inode);I think having inode_lock_shared(file_inode(memfd)) looks a bit more cleaner.
Good idea, changed that.
Also, wouldn't it be more appropriate here to take the writer's lock instead of the reader's lock given what we are doing (pinning) in udmabuf_create()?
I don't see why that would require taking the inode lock in write mode. I am taking the inode lock to provide exclusion against memfd_add_seals(), which uses inode_lock(); in other words, the inode_lock is to protect the sealing status of the file from changing (which is a reader-like requirement). I'll add a comment in v2 to clarify this.