linux-kernel - Re: [PATCH] proc: add missing '\0' back to /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]

Date:   Wed, 20 Jun 2018 07:08:24 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Alexey Dobriyan <adobriyan@...il.com>
Subject: Re: [PATCH] proc: add missing '\0' back to /proc/$pid/cmdline

On Wed, Jun 20, 2018 at 06:56:04AM +0900, Linus Torvalds wrote:
> On Wed, Jun 20, 2018 at 1:24 AM Michal Kubecek <mkubecek@...e.cz> wrote:
> >
> > Recent rewrite introduced a regression, /proc/$pid/cmdline is missing the
> > trailing null character:
> >
> > mike@...n:/tmp> cat /proc/self/cmdline | od -t c
> > 0000000   c   a   t  \0   /   p   r   o   c   /   s   e   l   f   /   c
> > 0000020   m   d   l   i   n   e
> > 0000026
> 
> Thanks, and obviously right you are.
> 
> That said, I'm not a fan of your patch. I'd much rather just tweak the
> "strnlen()" logic a bit instead, and make the rule be that when we go
> into the "slop" area, we always include the last byte of the "real"
> argv area.
> 
> That limits the slop to a page (well, one byte less, since we want the
> one byte of non-slop), but honestly, a page for *everything* was what
> we used to do originally, so..

Yes, that should be enough for real life applications.

> How does the attached patch work for you?

I haven't tested it yet but it looks good except this:

> @@ -254,10 +258,19 @@ static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
>  	while (count) {
>  		int got;
>  		size_t size = min_t(size_t, PAGE_SIZE, count);

We limit size to be at most PAGE_SIZE here.

> +		long offset;
>  
> -		got = access_remote_vm(mm, pos, page, size, FOLL_ANON);
> -		if (got <= 0)
> +		/*
> +		 * Are we already starting past the official end?
> +		 * We always include the last byte that is *supposed*
> +		 * to be NUL
> +		 */
> +		offset = (pos >= arg_end) ? pos - arg_end + 1 : 0;
> +
> +		got = access_remote_vm(mm, pos - offset, page, size + offset, FOLL_ANON);

But here we read (size + offset) bytes which may be more than PAGE_SIZE.
I guess it should rather be

	size_t size;
...
	offset = (pos >= arg_end) ? pos - arg_end + 1 : 0;
	size = min_t(size_t, PAGE_SIZE - offset, count);

We already made sure that offset < PAGE_SIZE so that size will be at
least 1.

Michal Kubecek