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: <20190716170339.1c44719d@gandalf.local.home>
Date:   Tue, 16 Jul 2019 17:03:39 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Qian Cai <cai@....pw>
Cc:     mingo@...hat.com, tj@...nel.org, dchinner@...hat.com,
        fengguang.wu@...el.com, jack@...e.cz, linux-kernel@...r.kernel.org,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH] trace/writeback: fix Wstringop-truncation warnings

On Fri, 12 Jul 2019 12:14:47 -0400
Qian Cai <cai@....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@....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;

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ