linux-kernel - Re: [PATCH 2/2] proc: fix PAGE_SIZE limit of /proc/$PID/cmdline

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: <20150527224825.GI17625@uranus.sw.swsoft.com>
Date:	Thu, 28 May 2015 01:48:25 +0300
From:	Cyrill Gorcunov <gorcunov@...il.com>
To:	Alexey Dobriyan <adobriyan@...il.com>
Cc:	akpm@...ux-foundation.org, linux-kernel@...r.kernel.org,
	jarod@...hat.com, jstancek@...hat.com
Subject: Re: [PATCH 2/2] proc: fix PAGE_SIZE limit of /proc/$PID/cmdline

On Thu, May 28, 2015 at 01:29:42AM +0300, Alexey Dobriyan wrote:
> > > +
> > > +	page = (char *)__get_free_page(GFP_TEMPORARY);
> > > +	if (!page) {
> > > +		rv = -ENOMEM;
> > > +		goto out_mmput;
> > > +	}
> > > +
> > > +	down_read(&mm->mmap_sem);
> > > +	arg_start = mm->arg_start;
> > > +	arg_end = mm->arg_end;
> > > +	env_start = mm->env_start;
> > > +	env_end = mm->env_end;
> > > +	up_read(&mm->mmap_sem);
> > 
> > Could you please explain why this down/up is needed?
> 
> Code is written this way to get constistent snapshot of data.

it does not. you fetch data into local variables which is the
same as simply read them locklessly in general (because later
you refer to local vars).

> If you look at PR_SET_MM_* code, you'll notice down_read(&mm->mmap_sem)
> as well which is a separate bug because you're _writing_ those fields
> eventually in prctl_set_mm(), yuck!

yes, there members are modified under read-lock and initially
i didn't see any problem with that except one can have inconsistent
statistics output because another process modified these fields
(we validate that new members are having sane values at least
 in new interface and after your first patch). But now I think
that down_write may be more suitable here.

	Cyrill
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/