| Message ID | 20241203-udmabuf-fixes-v1-1-f99281c345aa@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fixes for udmabuf (memfd sealing checks and a leak) | expand |
Hi Jann, > 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. 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()? Thanks, Vivek > ret = check_memfd_seals(memfd); > - if (ret < 0) { > - fput(memfd); > - goto err; > - } > + if (ret) > + goto out_unlock; > > ret = udmabuf_pin_folios(ubuf, memfd, list[i].offset, > list[i].size, folios); > +out_unlock: > + inode_unlock_shared(memfd->f_inode); > fput(memfd); > if (ret) > goto err; > > -- > 2.47.0.338.g60cca15819-goog
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.
diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c index 8ce1f074c2d32a0a9f59ff7184359e37d56548c6..662b9a26e06668bf59ab36d07c0648c7b02ee5ae 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); ret = check_memfd_seals(memfd); - if (ret < 0) { - fput(memfd); - goto err; - } + if (ret) + goto out_unlock; ret = udmabuf_pin_folios(ubuf, memfd, list[i].offset, list[i].size, folios); +out_unlock: + inode_unlock_shared(memfd->f_inode); fput(memfd); if (ret) goto err;
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=obrimeTgpD+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(-)