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] [thread-next>] [day] [month] [year] [list]
Message-ID: <dc30a97b-853e-4d2a-b171-e68fb3ab026c@lucifer.local>
Date:   Tue, 1 Aug 2023 17:33:18 +0100
From:   Lorenzo Stoakes <lstoakes@...il.com>
To:     David Hildenbrand <david@...hat.com>
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Baoquan He <bhe@...hat.com>,
        Uladzislau Rezki <urezki@...il.com>,
        linux-fsdevel@...r.kernel.org, Jiri Olsa <olsajiri@...il.com>,
        Will Deacon <will@...nel.org>, Mike Galbraith <efault@....de>,
        Mark Rutland <mark.rutland@....com>,
        wangkefeng.wang@...wei.com, catalin.marinas@....com,
        ardb@...nel.org,
        Linux regression tracking <regressions@...mhuis.info>,
        regressions@...ts.linux.dev, Matthew Wilcox <willy@...radead.org>,
        Liu Shixin <liushixin2@...wei.com>,
        Jens Axboe <axboe@...nel.dk>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        stable@...r.kernel.org
Subject: Re: [PATCH] fs/proc/kcore: reinstate bounce buffer for KCORE_TEXT
 regions

On Tue, Aug 01, 2023 at 11:05:40AM +0200, David Hildenbrand wrote:
> On 31.07.23 23:50, Lorenzo Stoakes wrote:
> > Some architectures do not populate the entire range categorised by
> > KCORE_TEXT, so we must ensure that the kernel address we read from is
> > valid.
> >
> > Unfortunately there is no solution currently available to do so with a
> > purely iterator solution so reinstate the bounce buffer in this instance so
> > we can use copy_from_kernel_nofault() in order to avoid page faults when
> > regions are unmapped.
> >
> > This change partly reverts commit 2e1c0170771e ("fs/proc/kcore: avoid
> > bounce buffer for ktext data"), reinstating the bounce buffer, but adapts
> > the code to continue to use an iterator.
> >
> > Fixes: 2e1c0170771e ("fs/proc/kcore: avoid bounce buffer for ktext data")
> > Reported-by: Jiri Olsa <olsajiri@...il.com>
> > Closes: https://lore.kernel.org/all/ZHc2fm+9daF6cgCE@krava
> > Cc: stable@...r.kernel.org
> > Signed-off-by: Lorenzo Stoakes <lstoakes@...il.com>
> > ---
> >   fs/proc/kcore.c | 26 +++++++++++++++++++++++++-
> >   1 file changed, 25 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
> > index 9cb32e1a78a0..3bc689038232 100644
> > --- a/fs/proc/kcore.c
> > +++ b/fs/proc/kcore.c
> > @@ -309,6 +309,8 @@ static void append_kcore_note(char *notes, size_t *i, const char *name,
> >   static ssize_t read_kcore_iter(struct kiocb *iocb, struct iov_iter *iter)
> >   {
> > +	struct file *file = iocb->ki_filp;
> > +	char *buf = file->private_data;
> >   	loff_t *fpos = &iocb->ki_pos;
> >   	size_t phdrs_offset, notes_offset, data_offset;
> >   	size_t page_offline_frozen = 1;
> > @@ -554,11 +556,22 @@ static ssize_t read_kcore_iter(struct kiocb *iocb, struct iov_iter *iter)
> >   			fallthrough;
> >   		case KCORE_VMEMMAP:
> >   		case KCORE_TEXT:
> > +			/*
> > +			 * Sadly we must use a bounce buffer here to be able to
> > +			 * make use of copy_from_kernel_nofault(), as these
> > +			 * memory regions might not always be mapped on all
> > +			 * architectures.
> > +			 */
> > +			if (copy_from_kernel_nofault(buf, (void *)start, tsz)) {
> > +				if (iov_iter_zero(tsz, iter) != tsz) {
> > +					ret = -EFAULT;
> > +					goto out;
> > +				}
> >   			/*
> >   			 * We use _copy_to_iter() to bypass usermode hardening
> >   			 * which would otherwise prevent this operation.
> >   			 */
>
> Having a comment at this indentation level looks for the else case looks
> kind of weird.

Yeah, but having it indented again would be weird and seem like it doesn't
apply to the block below, there's really no good spot for it and
checkpatch.pl doesn't mind so I think this is ok :)

>
> (does that comment still apply?)

Hm good point, actually, now we're using the bounce buffer we don't need to
avoid usermode hardening any more.

However since we've established a bounce buffer ourselves its still
appropriate to use _copy_to_iter() as we know the source region is good to
copy from.

To make life easy I'll just respin with an updated comment :)

>
>
> --
> Cheers,
>
> David / dhildenb
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ