On Fri, 16 Dec 2022 10:42:13 -0500
Steven Sistare <steven.sistare@xxxxxxxxxx> wrote:
> On 12/16/2022 9:09 AM, Jason Gunthorpe wrote:
> > On Thu, Dec 15, 2022 at 01:56:59PM -0800, Steve Sistare wrote:
> >> When a vfio container is preserved across exec, the task does not change,
> >> but it gets a new mm with locked_vm=0. If the user later unmaps a dma
> >> mapping, locked_vm underflows to a large unsigned value, and a subsequent
> >> dma map request fails with ENOMEM in __account_locked_vm.
> >>
> >> To avoid underflow, grab and save the mm at the time a dma is mapped.
> >> Use that mm when adjusting locked_vm, rather than re-acquiring the saved
> >> task's mm, which may have changed. If the saved mm is dead, do nothing.
> >>
> >> Signed-off-by: Steve Sistare <steven.sistare@xxxxxxxxxx>
> >> ---
> >> drivers/vfio/vfio_iommu_type1.c | 17 ++++++++++-------
> >> 1 file changed, 10 insertions(+), 7 deletions(-)
> >
> > Add fixes lines and a CC stable
>
> This predates the update vaddr functionality, so AFAICT:
>
> Fixes: 73fa0d10d077 ("vfio: Type1 IOMMU implementation")
>
> I'll wait on cc'ing stable until alex has chimed in.
Technically, adding the stable Cc tag is still the correct approach per
the stable process docs, but the Fixes: tag alone is generally
sufficient to crank up the backport engines. The original
implementation is probably the correct commit to identify, exec was
certainly not considered there. Thanks,
Alex
> > The subject should be more like 'vfio/typ1: Prevent corruption of mm->locked_vm via exec()'
>
> Underflow is a more precise description of the first corruption. How about:
>
> vfio/type1: Prevent underflow of locked_vm via exec()
>
> >> @@ -1687,6 +1689,8 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
> >> get_task_struct(current->group_leader);
> >> dma->task = current->group_leader;
> >> dma->lock_cap = capable(CAP_IPC_LOCK);
> >> + dma->mm = dma->task->mm;
> >
> > This should be current->mm, current->group_leader->mm is not quite the
> > same thing (and maybe another bug, I'm not sure)
>
> When are they different -- when the leader is a zombie?
>
> BTW I just noticed I need to update the comments about mm preceding these lines.
>
> - Steve
>
