RE: udmabuf: check_memfd_seals() is racy

RE: udmabuf: check_memfd_seals() is racy

[Kasireddy, Vivek]

Hi Jann, Julian,

> Subject: udmabuf: check_memfd_seals() is racy
> 
> Hi!
> 
> Julian Orth reported at
> https://bugzilla.kernel.org/show_bug.cgi?id=219106 that
Thank you for reporting this bug. 

> udmabuf_create() checks for F_SEAL_WRITE in a racy way, so a udmabuf
> can end up holding references to pages in a write-sealed memfd, which
> theoretically breaks one of the security properties of memfd sealing.
> See also the discussion starting at
> <https://lore.kernel.org/linux-
> mm/CAHijbEV6wtTQy01djSfWBJksq4AEoZ=KYUsaKEKNSXbTTSM-
> Ww@mail.gmail.com/>.
AFAICS, this problem does not adversely affect the main user of udmabuf driver
(Qemu) given that Qemu adds F_SEAL_SEAL while creating the memfd but
I can see how other users of udmabuf driver might be impacted by this issue.

> 
> I think one possible correct pattern would be something like:
> 
> mapping_map_writable() [with error bailout]
> check seals with F_GET_SEALS
> udmabuf_pin_folios()
> mapping_unmap_writable()
I believe this should probably work as mapping_map_writable() would prevent
F_SEAL_WRITE from getting added later. Do you plan to send a patch to fix
this issue in udmabuf driver?

Thanks,
Vivek

Re: udmabuf: check_memfd_seals() is racy

[Jann Horn]

On Tue, Dec 3, 2024 at 9:25 AM Kasireddy, Vivek
<vivek.kasireddy@intel.com> wrote:
> > Julian Orth reported at
> > https://bugzilla.kernel.org/show_bug.cgi?id=219106 that
> Thank you for reporting this bug.
>
> > udmabuf_create() checks for F_SEAL_WRITE in a racy way, so a udmabuf
> > can end up holding references to pages in a write-sealed memfd, which
> > theoretically breaks one of the security properties of memfd sealing.
> > See also the discussion starting at
> > <https://lore.kernel.org/linux-
> > mm/CAHijbEV6wtTQy01djSfWBJksq4AEoZ=KYUsaKEKNSXbTTSM-
> > Ww@mail.gmail.com/>.
> AFAICS, this problem does not adversely affect the main user of udmabuf driver
> (Qemu) given that Qemu adds F_SEAL_SEAL while creating the memfd but
> I can see how other users of udmabuf driver might be impacted by this issue.

The issue is that in theory, a nefarious process could maybe abuse
udmabuf to write to a memfd that is supposed to be sealed. This could
violate the assumption that a F_SEAL_WRITE-sealed memfd's memory is
immutable in another process. So the affected process wouldn't have to
be aware of udmabuf at all.

> > I think one possible correct pattern would be something like:
> >
> > mapping_map_writable() [with error bailout]
> > check seals with F_GET_SEALS
> > udmabuf_pin_folios()
> > mapping_unmap_writable()
> I believe this should probably work as mapping_map_writable() would prevent
> F_SEAL_WRITE from getting added later. Do you plan to send a patch to fix
> this issue in udmabuf driver?

Yes, I just sent fixes for this issue and two others at
<https://lore.kernel.org/all/20241203-udmabuf-fixes-v1-0-f99281c345aa@google.com/>
(though I went with the inode lock instead of mapping_map_writable()
to keep things simpler).