Re: [PATCH v2] pstore: pstore_ftrace_seq_next should increase position index

[Kees Cook <keescook@chromium.org>]

[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@vger.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@virtuozzo.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