linux-kernel - Re: [PATCH v3] comedi: Flush partial mappings in error case

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAG48ez3oaXHQ=7Ys+-RkAaUTsPofKr13RbGrAkxOLGqQrrDf4w@mail.gmail.com>
Date: Fri, 18 Oct 2024 17:33:57 +0200
From: Jann Horn <jannh@...gle.com>
To: Ian Abbott <abbotti@....co.uk>
Cc: H Hartley Sweeten <hsweeten@...ionengravers.com>, Frank Mori Hess <fmh6jj@...il.com>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, linux-kernel@...r.kernel.org, 
	stable@...r.kernel.org
Subject: Re: [PATCH v3] comedi: Flush partial mappings in error case

On Fri, Oct 18, 2024 at 1:35 PM Ian Abbott <abbotti@....co.uk> wrote:
> On 17/10/2024 20:07, Jann Horn wrote:
> > If some remap_pfn_range() calls succeeded before one failed, we still have
> > buffer pages mapped into the userspace page tables when we drop the buffer
> > reference with comedi_buf_map_put(bm). The userspace mappings are only
> > cleaned up later in the mmap error path.
> >
> > Fix it by explicitly flushing all mappings in our VMA on the error path.
> >
> > See commit 79a61cc3fc04 ("mm: avoid leaving partial pfn mappings around in
> > error case").
> >
> > Cc: stable@...r.kernel.org
> > Fixes: ed9eccbe8970 ("Staging: add comedi core")
> > Signed-off-by: Jann Horn <jannh@...gle.com>
> > ---
> > Note: compile-tested only; I don't actually have comedi hardware, and I
> > don't know anything about comedi.
> > ---
> > Changes in v3:
> > - gate zapping ptes on CONFIG_MMU (Intel kernel test robot)
> > - Link to v2: https://lore.kernel.org/r/20241015-comedi-tlb-v2-1-cafb0e27dd9a@google.com
> >
> > Changes in v2:
> > - only do the zapping in the pfnmap path (Ian Abbott)
> > - use zap_vma_ptes() instead of zap_page_range_single() (Ian Abbott)
> > - Link to v1: https://lore.kernel.org/r/20241014-comedi-tlb-v1-1-4b699144b438@google.com
> > ---
> >   drivers/comedi/comedi_fops.c | 12 ++++++++++++
> >   1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/comedi/comedi_fops.c b/drivers/comedi/comedi_fops.c
> > index 1b481731df96..b9df9b19d4bd 100644
> > --- a/drivers/comedi/comedi_fops.c
> > +++ b/drivers/comedi/comedi_fops.c
> > @@ -2407,6 +2407,18 @@ static int comedi_mmap(struct file *file, struct vm_area_struct *vma)
> >
> >                       start += PAGE_SIZE;
> >               }
> > +
> > +#ifdef CONFIG_MMU
> > +             /*
> > +              * Leaving behind a partial mapping of a buffer we're about to
> > +              * drop is unsafe, see remap_pfn_range_notrack().
> > +              * We need to zap the range here ourselves instead of relying
> > +              * on the automatic zapping in remap_pfn_range() because we call
> > +              * remap_pfn_range() in a loop.
> > +              */
> > +             if (retval)
>
> Perhaps that condition should be changed to `retval && i` since there
> will be no partial mappings left behind if the first call to
> `remap_pfn_range` failed.

I'll add that if you really want, but I think it just makes the code
harder to follow if you have to think about how the code paths differ
between "we have done at least one successful iteration and failed
later" vs "the first iteration failed"...