linux-kernel - Re: [PATCH v2] pstore: pstore_ftrace_seq

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: <202002251136.3816A79E@keescook>
Date:   Tue, 25 Feb 2020 11:39:48 -0800
From:   Kees Cook <keescook@...omium.org>
To:     Vasily Averin <vvs@...tuozzo.com>, joelaf@...gle.com
Cc:     linux-kernel@...r.kernel.org, Anton Vorontsov <anton@...msg.org>,
        Colin Cross <ccross@...roid.com>,
        Tony Luck <tony.luck@...el.com>
Subject: Re: [PATCH v2] pstore: pstore_ftrace_seq_next should increase
 position index

[merged threads]

On Tue, Feb 25, 2020 at 11:11:20AM +0300, Vasily Averin wrote:
> In Aug 2018 NeilBrown noticed 
> commit 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code and interface")
> "Some ->next functions do not increment *pos when they return NULL...
> Note that such ->next functions are buggy and should be fixed. 
> A simple demonstration is
> 
> dd if=/proc/swaps bs=1000 skip=1
> 
> Choose any block size larger than the size of /proc/swaps.  This will
> always show the whole last line of /proc/swaps"
> 
> /proc/swaps output was fixed recently, however there are lot of other
> affected files, and one of them is related to pstore subsystem.
> 
> If .next function does not change position index, following .show function
> will repeat output related to current position index.
> 
> There are at least 2 related problems:
> - read after lseek beyond end of file, described above by NeilBrown
>   "dd if=<AFFECTED_FILE> bs=1000 skip=1" will generate whole last list
> - read after lseek on in middle of last line will output expected rest of
>   last line but then repeat whole last line once again.
> 
> If .show() function generates multy-line output
> (like pstore_ftrace_seq_show() does ?)
> following bash script cycles endlessly
> 
>  $ q=;while read -r r;do echo "$((++q)) $r";done < AFFECTED_FILE
> 
> Unfortunately I'm not familiar enough to pstore subsystem and was unable to
> find affected pstore-related file on my test node.
> 
> If .next function does not change position index,
> following .show function will repeat output related
> to current position index.
> 
> Cc: stable@...r.kernel.org
> Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code ...")
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206283
> Signed-off-by: Vasily Averin <vvs@...tuozzo.com>
> ---
>  fs/pstore/inode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
> index 7fbe8f0..ea8799b 100644
> --- a/fs/pstore/inode.c
> +++ b/fs/pstore/inode.c
> @@ -87,11 +87,11 @@ static void *pstore_ftrace_seq_next(struct seq_file *s, void *v, loff_t *pos)
>  	struct pstore_private *ps = s->private;
>  	struct pstore_ftrace_seq_data *data = v;
>  
> +	(*pos)++;
>  	data->off += REC_SIZE;
>  	if (data->off + REC_SIZE > ps->total_size)
>  		return NULL;
>  
> -	(*pos)++;
>  	return data;
>  }
>  
> -- 
> 1.8.3.1
> 

I think this make sense, but I figured I'd check with Joel first. Does
this look sane for how ftrace will merge records?

-- 
Kees Cook