linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Steven Rostedt <rostedt@goodmis.org>
To: Qian Cai <cai@lca.pw>
Cc: mingo@redhat.com, tj@kernel.org, dchinner@redhat.com,
	fengguang.wu@intel.com, jack@suse.cz,
	linux-kernel@vger.kernel.org,
	Peter Zijlstra <peterz@infradead.org>
Subject: Re: [PATCH] trace/writeback: fix Wstringop-truncation warnings
Date: Tue, 16 Jul 2019 17:03:39 -0400	[thread overview]
Message-ID: <20190716170339.1c44719d@gandalf.local.home> (raw)
In-Reply-To: <1562948087-5374-1-git-send-email-cai@lca.pw>

On Fri, 12 Jul 2019 12:14:47 -0400
Qian Cai <cai@lca.pw> wrote:

> There are many of those warnings.
> 
> In file included from ./arch/powerpc/include/asm/paca.h:15,
>                  from ./arch/powerpc/include/asm/current.h:13,
>                  from ./include/linux/thread_info.h:21,
>                  from ./include/asm-generic/preempt.h:5,
>                  from ./arch/powerpc/include/generated/asm/preempt.h:1,
>                  from ./include/linux/preempt.h:78,
>                  from ./include/linux/spinlock.h:51,
>                  from fs/fs-writeback.c:19:
> In function 'strncpy',
>     inlined from 'perf_trace_writeback_page_template' at
> ./include/trace/events/writeback.h:56:1:
> ./include/linux/string.h:260:9: warning: '__builtin_strncpy' specified
> bound 32 equals destination size [-Wstringop-truncation]
>   return __builtin_strncpy(p, q, size);
>          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Fix it by using strlcpy() which will always be NUL-terminated instead of
> strncpy(). strlcpy() has already been used at some places in this file.
> 
> Fixes: 455b2864686d ("writeback: Initial tracing support")
> Fixes: 028c2dd184c0 ("writeback: Add tracing to balance_dirty_pages")
> Fixes: e84d0a4f8e39 ("writeback: trace event writeback_queue_io")
> Fixes: b48c104d2211 ("writeback: trace event bdi_dirty_ratelimit")
> Fixes: cc1676d917f3 ("writeback: Move requeueing when I_SYNC set to writeback_sb_inodes()")
> Fixes: 9fb0a7da0c52 ("writeback: add more tracepoints")
> 
> Signed-off-by: Qian Cai <cai@lca.pw>
> ---
>  include/trace/events/writeback.h | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/include/trace/events/writeback.h b/include/trace/events/writeback.h
> index aa7f3aeac740..8e3b3c4fd964 100644
> --- a/include/trace/events/writeback.h
> +++ b/include/trace/events/writeback.h
> @@ -66,7 +66,7 @@
>  	),
>  
>  	TP_fast_assign(
> -		strncpy(__entry->name,
> +		strlcpy(__entry->name,
>  			mapping ? dev_name(inode_to_bdi(mapping->host)->dev) : "(unknown)", 32);


Not sure this is an issue or not, but although the fix looks legit (in
case a string is more that 31 bytes), strlcpy() does not pad the rest
of the string like strncpy() does. This means we can possibly leak data
through the ring buffer.

This may not be an issue as ftrace can only be used by a super user
account, but this code can also be used by perf. If it is possible for
a non admin account to enable these events through perf, then there is
a case of data leak.

Again, it may not be a big issue, but I'm just letting people know.

Note, this needs to go through the maintainer of the writeback.h, who
are those that created it, not the tracing maintainers.

-- Steve


>  		__entry->ino = mapping ? mapping->host->i_ino : 0;
>  		__entry->index = page->index;
> @@ -110,7 +110,7 @@
>  		struct backing_dev_info *bdi = inode_to_bdi(inode);
>  
>  		/* may be called for files on pseudo FSes w/ unregistered bdi */
> -		strncpy(__entry->name,
> +		strlcpy(__entry->name,
>  			bdi->dev ? dev_name(bdi->dev) : "(unknown)", 32);
>  		__entry->ino		= inode->i_ino;
>  		__entry->state		= inode->i_state;
> @@ -190,7 +190,7 @@ static inline unsigned int __trace_wbc_assign_cgroup(struct writeback_control *w
>  	),
>  
>  	TP_fast_assign(
> -		strncpy(__entry->name,
> +		strlcpy(__entry->name,
>  			dev_name(inode_to_bdi(inode)->dev), 32);
>  		__entry->ino		= inode->i_ino;
>  		__entry->sync_mode	= wbc->sync_mode;
> @@ -234,7 +234,7 @@ static inline unsigned int __trace_wbc_assign_cgroup(struct writeback_control *w
>  		__field(unsigned int, cgroup_ino)
>  	),
>  	TP_fast_assign(
> -		strncpy(__entry->name,
> +		strlcpy(__entry->name,
>  			wb->bdi->dev ? dev_name(wb->bdi->dev) : "(unknown)", 32);
>  		__entry->nr_pages = work->nr_pages;
>  		__entry->sb_dev = work->sb ? work->sb->s_dev : 0;
> @@ -288,7 +288,7 @@ static inline unsigned int __trace_wbc_assign_cgroup(struct writeback_control *w
>  		__field(unsigned int, cgroup_ino)
>  	),
>  	TP_fast_assign(
> -		strncpy(__entry->name, dev_name(wb->bdi->dev), 32);
> +		strlcpy(__entry->name, dev_name(wb->bdi->dev), 32);
>  		__entry->cgroup_ino = __trace_wb_assign_cgroup(wb);
>  	),
>  	TP_printk("bdi %s: cgroup_ino=%u",
> @@ -310,7 +310,7 @@ static inline unsigned int __trace_wbc_assign_cgroup(struct writeback_control *w
>  		__array(char, name, 32)
>  	),
>  	TP_fast_assign(
> -		strncpy(__entry->name, dev_name(bdi->dev), 32);
> +		strlcpy(__entry->name, dev_name(bdi->dev), 32);
>  	),
>  	TP_printk("bdi %s",
>  		__entry->name
> @@ -335,7 +335,7 @@ static inline unsigned int __trace_wbc_assign_cgroup(struct writeback_control *w
>  	),
>  
>  	TP_fast_assign(
> -		strncpy(__entry->name, dev_name(bdi->dev), 32);
> +		strlcpy(__entry->name, dev_name(bdi->dev), 32);
>  		__entry->nr_to_write	= wbc->nr_to_write;
>  		__entry->pages_skipped	= wbc->pages_skipped;
>  		__entry->sync_mode	= wbc->sync_mode;
> @@ -386,7 +386,7 @@ static inline unsigned int __trace_wbc_assign_cgroup(struct writeback_control *w
>  	),
>  	TP_fast_assign(
>  		unsigned long *older_than_this = work->older_than_this;
> -		strncpy(__entry->name, dev_name(wb->bdi->dev), 32);
> +		strlcpy(__entry->name, dev_name(wb->bdi->dev), 32);
>  		__entry->older	= older_than_this ?  *older_than_this : 0;
>  		__entry->age	= older_than_this ?
>  				  (jiffies - *older_than_this) * 1000 / HZ : -1;
> @@ -597,7 +597,7 @@ static inline unsigned int __trace_wbc_assign_cgroup(struct writeback_control *w
>  	),
>  
>  	TP_fast_assign(
> -		strncpy(__entry->name,
> +		strlcpy(__entry->name,
>  		        dev_name(inode_to_bdi(inode)->dev), 32);
>  		__entry->ino		= inode->i_ino;
>  		__entry->state		= inode->i_state;
> @@ -671,7 +671,7 @@ static inline unsigned int __trace_wbc_assign_cgroup(struct writeback_control *w
>  	),
>  
>  	TP_fast_assign(
> -		strncpy(__entry->name,
> +		strlcpy(__entry->name,
>  			dev_name(inode_to_bdi(inode)->dev), 32);
>  		__entry->ino		= inode->i_ino;
>  		__entry->state		= inode->i_state;


  reply	other threads:[~2019-07-16 21:03 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-12 16:14 Qian Cai
2019-07-16 21:03 ` Steven Rostedt [this message]
2019-07-18 20:11   ` Qian Cai
2019-07-29 10:04   ` Jan Kara

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190716170339.1c44719d@gandalf.local.home \
    --to=rostedt@goodmis.org \
    --cc=cai@lca.pw \
    --cc=dchinner@redhat.com \
    --cc=fengguang.wu@intel.com \
    --cc=jack@suse.cz \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    --cc=tj@kernel.org \
    --subject='Re: [PATCH] trace/writeback: fix Wstringop-truncation warnings' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).