[PATCH 0/2] vsprintf: ignore %n again

[PATCH 0/2] vsprintf: ignore %n again

[Kees Cook]

Whether seq_printf should return void or error, %n still needs to be removed.
As such, instead of changing the seq_file structure and adding instructions
to all callers of seq_printf, just examine seq->count for the callers that
care about how many characters were put into the buffer, as suggested by
George Spelvin. First patch removes all %n usage in favor of checking
seq->count before/after. Second patch makes %n ignore its argument.

Testing shows this all works happily, and everything is still getting
padded correctly:

/proc/consoles:
ttyS0                -W- (EC   a)    4:64
netcon0              -W- (E     )

/proc/self/maps:
...
01ee7000-01f08000 rw-p 00000000 00:00 0                                  [heap]
7fdc79bd4000-7fdc79bf6000 r-xp 00000000 fd:01 394247                     /lib/x86_64-linux-gnu/libtinfo.so.5.9
...

/proc/net/tcp
  sl  local_address rem_address   st tx_queue rx_queue tr tm->when retrnsmt   uid  timeout inode
   0: 00000000:0016 00000000:0000 0A 00000000:00000000 00:00000000 00000000     0        0 2239 1 ffff88007bfd0000 100 0 0 10 0
...

/proc/net/udp
  sl  local_address rem_address   st tx_queue rx_queue tr tm->when retrnsmt   uid  timeout inode ref pointer drops
   12: 0DAAA8C0:D9D1 0100000A:0035 01 00000000:00000000 00:00000000 00000000     0        0 7534 2 ffff880078048000 0


And a test with a %n in a format string shows the warning:

[   10.693638] ------------[ cut here ]------------
[   10.693657] WARNING: CPU: 0 PID: 2048 at lib/vsprintf.c:1693 vsnprintf+0x5c1/0x600()
[   10.693660] Please remove ignored %n in '%n
[   10.693663] '
...

Fixing the other callers of seq_printf to do the right thing (void or not)
can be separate from this series.

-Kees

[PATCH 1/2] remove all uses of printf's %n

[Kees Cook]

All users of %n are calculating padding size when using seq_file, so
instead use the new last_len member for discovering the length of the
written strings.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/proc/consoles.c   |    5 +++--
 fs/proc/nommu.c      |    6 ++++--
 fs/proc/task_mmu.c   |    6 ++++--
 fs/proc/task_nommu.c |    6 ++++--
 net/ipv4/fib_trie.c  |   10 ++++++----
 net/ipv4/ping.c      |   10 ++++++----
 net/ipv4/tcp_ipv4.c  |   28 ++++++++++++++--------------
 net/ipv4/udp.c       |   10 ++++++----
 net/phonet/socket.c  |   18 +++++++++++-------
 net/sctp/objcnt.c    |    6 ++++--
 10 files changed, 62 insertions(+), 43 deletions(-)

diff --git a/fs/proc/consoles.c b/fs/proc/consoles.c
index b701eaa..08cd2ee 100644
--- a/fs/proc/consoles.c
+++ b/fs/proc/consoles.c
@@ -47,8 +47,9 @@ static int show_console_dev(struct seq_file *m, void *v)
 			con_flags[a].name : ' ';
 	flags[a] = 0;
 
-	seq_printf(m, "%s%d%n", con->name, con->index, &len);
-	len = 21 - len;
+	len = m->count;
+	seq_printf(m, "%s%d", con->name, con->index);
+	len = 21 - (m->count - len);
 	if (len < 1)
 		len = 1;
 	seq_printf(m, "%*c%c%c%c (%s)", len, ' ', con->read ? 'R' : '-',
diff --git a/fs/proc/nommu.c b/fs/proc/nommu.c
index ccfd99b..db0700d 100644
--- a/fs/proc/nommu.c
+++ b/fs/proc/nommu.c
@@ -50,8 +50,9 @@ static int nommu_region_show(struct seq_file *m, struct vm_region *region)
 		ino = inode->i_ino;
 	}
 
+	len = m->count;
 	seq_printf(m,
-		   "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu %n",
+		   "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu ",
 		   region->vm_start,
 		   region->vm_end,
 		   flags & VM_READ ? 'r' : '-',
@@ -59,7 +60,8 @@ static int nommu_region_show(struct seq_file *m, struct vm_region *region)
 		   flags & VM_EXEC ? 'x' : '-',
 		   flags & VM_MAYSHARE ? flags & VM_SHARED ? 'S' : 's' : 'p',
 		   ((loff_t)region->vm_pgoff) << PAGE_SHIFT,
-		   MAJOR(dev), MINOR(dev), ino, &len);
+		   MAJOR(dev), MINOR(dev), ino);
+	len = m->count - len;
 
 	if (file) {
 		len = 25 + sizeof(void *) * 6 - len;
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 7366e9d..eaca69c 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -286,7 +286,8 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma, int is_pid)
 	if (stack_guard_page_end(vma, end))
 		end -= PAGE_SIZE;
 
-	seq_printf(m, "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu %n",
+	len = m->count;
+	seq_printf(m, "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu ",
 			start,
 			end,
 			flags & VM_READ ? 'r' : '-',
@@ -294,7 +295,8 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma, int is_pid)
 			flags & VM_EXEC ? 'x' : '-',
 			flags & VM_MAYSHARE ? 's' : 'p',
 			pgoff,
-			MAJOR(dev), MINOR(dev), ino, &len);
+			MAJOR(dev), MINOR(dev), ino);
+	len = m->count - len;
 
 	/*
 	 * Print the dentry name for named mappings, and a
diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
index 56123a6..04c8246 100644
--- a/fs/proc/task_nommu.c
+++ b/fs/proc/task_nommu.c
@@ -155,8 +155,9 @@ static int nommu_vma_show(struct seq_file *m, struct vm_area_struct *vma,
 		pgoff = (loff_t)vma->vm_pgoff << PAGE_SHIFT;
 	}
 
+	len = m->count;
 	seq_printf(m,
-		   "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu %n",
+		   "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu ",
 		   vma->vm_start,
 		   vma->vm_end,
 		   flags & VM_READ ? 'r' : '-',
@@ -164,7 +165,8 @@ static int nommu_vma_show(struct seq_file *m, struct vm_area_struct *vma,
 		   flags & VM_EXEC ? 'x' : '-',
 		   flags & VM_MAYSHARE ? flags & VM_SHARED ? 'S' : 's' : 'p',
 		   pgoff,
-		   MAJOR(dev), MINOR(dev), ino, &len);
+		   MAJOR(dev), MINOR(dev), ino);
+	len = m->count - len;
 
 	if (file) {
 		pad_len_spaces(m, len);
diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
index 3df6d3e..9537a95 100644
--- a/net/ipv4/fib_trie.c
+++ b/net/ipv4/fib_trie.c
@@ -2536,10 +2536,11 @@ static int fib_route_seq_show(struct seq_file *seq, void *v)
 			    || fa->fa_type == RTN_MULTICAST)
 				continue;
 
+			len = seq->count;
 			if (fi)
 				seq_printf(seq,
 					 "%s\t%08X\t%08X\t%04X\t%d\t%u\t"
-					 "%d\t%08X\t%d\t%u\t%u%n",
+					 "%d\t%08X\t%d\t%u\t%u",
 					 fi->fib_dev ? fi->fib_dev->name : "*",
 					 prefix,
 					 fi->fib_nh->nh_gw, flags, 0, 0,
@@ -2548,13 +2549,14 @@ static int fib_route_seq_show(struct seq_file *seq, void *v)
 					 (fi->fib_advmss ?
 					  fi->fib_advmss + 40 : 0),
 					 fi->fib_window,
-					 fi->fib_rtt >> 3, &len);
+					 fi->fib_rtt >> 3);
 			else
 				seq_printf(seq,
 					 "*\t%08X\t%08X\t%04X\t%d\t%u\t"
-					 "%d\t%08X\t%d\t%u\t%u%n",
+					 "%d\t%08X\t%d\t%u\t%u",
 					 prefix, 0, flags, 0, 0, 0,
-					 mask, 0, 0, 0, &len);
+					 mask, 0, 0, 0);
+			len = seq->count - len;
 
 			seq_printf(seq, "%*s\n", 127 - len, "");
 		}
diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index d7d9882..c87d062 100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -1073,7 +1073,7 @@ void ping_seq_stop(struct seq_file *seq, void *v)
 EXPORT_SYMBOL_GPL(ping_seq_stop);
 
 static void ping_v4_format_sock(struct sock *sp, struct seq_file *f,
-		int bucket, int *len)
+		int bucket)
 {
 	struct inet_sock *inet = inet_sk(sp);
 	__be32 dest = inet->inet_daddr;
@@ -1082,7 +1082,7 @@ static void ping_v4_format_sock(struct sock *sp, struct seq_file *f,
 	__u16 srcp = ntohs(inet->inet_sport);
 
 	seq_printf(f, "%5d: %08X:%04X %08X:%04X"
-		" %02X %08X:%08X %02X:%08lX %08X %5u %8d %lu %d %pK %d%n",
+		" %02X %08X:%08X %02X:%08lX %08X %5u %8d %lu %d %pK %d",
 		bucket, src, srcp, dest, destp, sp->sk_state,
 		sk_wmem_alloc_get(sp),
 		sk_rmem_alloc_get(sp),
@@ -1090,7 +1090,7 @@ static void ping_v4_format_sock(struct sock *sp, struct seq_file *f,
 		from_kuid_munged(seq_user_ns(f), sock_i_uid(sp)),
 		0, sock_i_ino(sp),
 		atomic_read(&sp->sk_refcnt), sp,
-		atomic_read(&sp->sk_drops), len);
+		atomic_read(&sp->sk_drops));
 }
 
 static int ping_v4_seq_show(struct seq_file *seq, void *v)
@@ -1104,7 +1104,9 @@ static int ping_v4_seq_show(struct seq_file *seq, void *v)
 		struct ping_iter_state *state = seq->private;
 		int len;
 
-		ping_v4_format_sock(v, seq, state->bucket, &len);
+		len = seq->count;
+		ping_v4_format_sock(v, seq, state->bucket);
+		len = seq->count - len;
 		seq_printf(seq, "%*s\n", 127 - len, "");
 	}
 	return 0;
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index b14266b..cdded47 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -2598,13 +2598,13 @@ void tcp_proc_unregister(struct net *net, struct tcp_seq_afinfo *afinfo)
 EXPORT_SYMBOL(tcp_proc_unregister);
 
 static void get_openreq4(const struct sock *sk, const struct request_sock *req,
-			 struct seq_file *f, int i, kuid_t uid, int *len)
+			 struct seq_file *f, int i, kuid_t uid)
 {
 	const struct inet_request_sock *ireq = inet_rsk(req);
 	long delta = req->expires - jiffies;
 
 	seq_printf(f, "%4d: %08X:%04X %08X:%04X"
-		" %02X %08X:%08X %02X:%08lX %08X %5u %8d %u %d %pK%n",
+		" %02X %08X:%08X %02X:%08lX %08X %5u %8d %u %d %pK",
 		i,
 		ireq->loc_addr,
 		ntohs(inet_sk(sk)->inet_sport),
@@ -2619,11 +2619,10 @@ static void get_openreq4(const struct sock *sk, const struct request_sock *req,
 		0,  /* non standard timer */
 		0, /* open_requests have no inode */
 		atomic_read(&sk->sk_refcnt),
-		req,
-		len);
+		req);
 }
 
-static void get_tcp4_sock(struct sock *sk, struct seq_file *f, int i, int *len)
+static void get_tcp4_sock(struct sock *sk, struct seq_file *f, int i)
 {
 	int timer_active;
 	unsigned long timer_expires;
@@ -2662,7 +2661,7 @@ static void get_tcp4_sock(struct sock *sk, struct seq_file *f, int i, int *len)
 		rx_queue = max_t(int, tp->rcv_nxt - tp->copied_seq, 0);
 
 	seq_printf(f, "%4d: %08X:%04X %08X:%04X %02X %08X:%08X %02X:%08lX "
-			"%08X %5u %8d %lu %d %pK %lu %lu %u %u %d%n",
+			"%08X %5u %8d %lu %d %pK %lu %lu %u %u %d",
 		i, src, srcp, dest, destp, sk->sk_state,
 		tp->write_seq - tp->snd_una,
 		rx_queue,
@@ -2679,12 +2678,11 @@ static void get_tcp4_sock(struct sock *sk, struct seq_file *f, int i, int *len)
 		tp->snd_cwnd,
 		sk->sk_state == TCP_LISTEN ?
 		    (fastopenq ? fastopenq->max_qlen : 0) :
-		    (tcp_in_initial_slowstart(tp) ? -1 : tp->snd_ssthresh),
-		len);
+		    (tcp_in_initial_slowstart(tp) ? -1 : tp->snd_ssthresh));
 }
 
 static void get_timewait4_sock(const struct inet_timewait_sock *tw,
-			       struct seq_file *f, int i, int *len)
+			       struct seq_file *f, int i)
 {
 	__be32 dest, src;
 	__u16 destp, srcp;
@@ -2696,10 +2694,10 @@ static void get_timewait4_sock(const struct inet_timewait_sock *tw,
 	srcp  = ntohs(tw->tw_sport);
 
 	seq_printf(f, "%4d: %08X:%04X %08X:%04X"
-		" %02X %08X:%08X %02X:%08lX %08X %5d %8d %d %d %pK%n",
+		" %02X %08X:%08X %02X:%08lX %08X %5d %8d %d %d %pK",
 		i, src, srcp, dest, destp, tw->tw_substate, 0, 0,
 		3, jiffies_delta_to_clock_t(delta), 0, 0, 0, 0,
-		atomic_read(&tw->tw_refcnt), tw, len);
+		atomic_read(&tw->tw_refcnt), tw);
 }
 
 #define TMPSZ 150
@@ -2718,18 +2716,20 @@ static int tcp4_seq_show(struct seq_file *seq, void *v)
 	}
 	st = seq->private;
 
+	len = seq->count;
 	switch (st->state) {
 	case TCP_SEQ_STATE_LISTENING:
 	case TCP_SEQ_STATE_ESTABLISHED:
-		get_tcp4_sock(v, seq, st->num, &len);
+		get_tcp4_sock(v, seq, st->num);
 		break;
 	case TCP_SEQ_STATE_OPENREQ:
-		get_openreq4(st->syn_wait_sk, v, seq, st->num, st->uid, &len);
+		get_openreq4(st->syn_wait_sk, v, seq, st->num, st->uid);
 		break;
 	case TCP_SEQ_STATE_TIME_WAIT:
-		get_timewait4_sock(v, seq, st->num, &len);
+		get_timewait4_sock(v, seq, st->num);
 		break;
 	}
+	len = seq->count - len;
 	seq_printf(seq, "%*s\n", TMPSZ - 1 - len, "");
 out:
 	return 0;
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 74d2c95..5460ab9 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -2150,7 +2150,7 @@ EXPORT_SYMBOL(udp_proc_unregister);
 
 /* ------------------------------------------------------------------------ */
 static void udp4_format_sock(struct sock *sp, struct seq_file *f,
-		int bucket, int *len)
+		int bucket)
 {
 	struct inet_sock *inet = inet_sk(sp);
 	__be32 dest = inet->inet_daddr;
@@ -2159,7 +2159,7 @@ static void udp4_format_sock(struct sock *sp, struct seq_file *f,
 	__u16 srcp	  = ntohs(inet->inet_sport);
 
 	seq_printf(f, "%5d: %08X:%04X %08X:%04X"
-		" %02X %08X:%08X %02X:%08lX %08X %5u %8d %lu %d %pK %d%n",
+		" %02X %08X:%08X %02X:%08lX %08X %5u %8d %lu %d %pK %d",
 		bucket, src, srcp, dest, destp, sp->sk_state,
 		sk_wmem_alloc_get(sp),
 		sk_rmem_alloc_get(sp),
@@ -2167,7 +2167,7 @@ static void udp4_format_sock(struct sock *sp, struct seq_file *f,
 		from_kuid_munged(seq_user_ns(f), sock_i_uid(sp)),
 		0, sock_i_ino(sp),
 		atomic_read(&sp->sk_refcnt), sp,
-		atomic_read(&sp->sk_drops), len);
+		atomic_read(&sp->sk_drops));
 }
 
 int udp4_seq_show(struct seq_file *seq, void *v)
@@ -2181,7 +2181,9 @@ int udp4_seq_show(struct seq_file *seq, void *v)
 		struct udp_iter_state *state = seq->private;
 		int len;
 
-		udp4_format_sock(v, seq, state->bucket, &len);
+		len = seq->count;
+		udp4_format_sock(v, seq, state->bucket);
+		len = seq->count - len;
 		seq_printf(seq, "%*s\n", 127 - len, "");
 	}
 	return 0;
diff --git a/net/phonet/socket.c b/net/phonet/socket.c
index 77e38f7..b8712ad 100644
--- a/net/phonet/socket.c
+++ b/net/phonet/socket.c
@@ -597,23 +597,25 @@ static int pn_sock_seq_show(struct seq_file *seq, void *v)
 {
 	int len;
 
+	len = seq->count;
 	if (v == SEQ_START_TOKEN)
-		seq_printf(seq, "%s%n", "pt  loc  rem rs st tx_queue rx_queue "
-			"  uid inode ref pointer drops", &len);
+		seq_puts(seq, "pt  loc  rem rs st tx_queue rx_queue "
+			"  uid inode ref pointer drops");
 	else {
 		struct sock *sk = v;
 		struct pn_sock *pn = pn_sk(sk);
 
 		seq_printf(seq, "%2d %04X:%04X:%02X %02X %08X:%08X %5d %lu "
-			"%d %pK %d%n",
+			"%d %pK %d",
 			sk->sk_protocol, pn->sobject, pn->dobject,
 			pn->resource, sk->sk_state,
 			sk_wmem_alloc_get(sk), sk_rmem_alloc_get(sk),
 			from_kuid_munged(seq_user_ns(seq), sock_i_uid(sk)),
 			sock_i_ino(sk),
 			atomic_read(&sk->sk_refcnt), sk,
-			atomic_read(&sk->sk_drops), &len);
+			atomic_read(&sk->sk_drops));
 	}
+	len = seq->count - len;
 	seq_printf(seq, "%*s\n", 127 - len, "");
 	return 0;
 }
@@ -787,17 +789,19 @@ static int pn_res_seq_show(struct seq_file *seq, void *v)
 {
 	int len;
 
+	len = seq->count;
 	if (v == SEQ_START_TOKEN)
-		seq_printf(seq, "%s%n", "rs   uid inode", &len);
+		seq_puts(seq, "rs   uid inode");
 	else {
 		struct sock **psk = v;
 		struct sock *sk = *psk;
 
-		seq_printf(seq, "%02X %5u %lu%n",
+		seq_printf(seq, "%02X %5u %lu",
 			   (int) (psk - pnres.sk),
 			   from_kuid_munged(seq_user_ns(seq), sock_i_uid(sk)),
-			   sock_i_ino(sk), &len);
+			   sock_i_ino(sk));
 	}
+	len = seq->count - len;
 	seq_printf(seq, "%*s\n", 63 - len, "");
 	return 0;
 }
diff --git a/net/sctp/objcnt.c b/net/sctp/objcnt.c
index 5ea573b..b3f4a3c 100644
--- a/net/sctp/objcnt.c
+++ b/net/sctp/objcnt.c
@@ -82,8 +82,10 @@ static int sctp_objcnt_seq_show(struct seq_file *seq, void *v)
 	int i, len;
 
 	i = (int)*(loff_t *)v;
-	seq_printf(seq, "%s: %d%n", sctp_dbg_objcnt[i].label,
-				atomic_read(sctp_dbg_objcnt[i].counter), &len);
+	len = seq->count;
+	seq_printf(seq, "%s: %d", sctp_dbg_objcnt[i].label,
+				atomic_read(sctp_dbg_objcnt[i].counter));
+	len = seq->count - len;
 	seq_printf(seq, "%*s\n", 127 - len, "");
 	return 0;
 }
-- 
1.7.9.5

Re: [PATCH 1/2] remove all uses of printf's %n

[Geert Uytterhoeven]

On Mon, Sep 16, 2013 at 9:43 AM, Kees Cook <keescook@chromium.org> wrote:
> All users of %n are calculating padding size when using seq_file, so
> instead use the new last_len member for discovering the length of the
> written strings.

Would it make sense to provide a seq_pad(...) function instead, to avoid
exposing more seq_file internals to its callers?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/2] remove all uses of printf's %n

[Kees Cook]

On Mon, Sep 16, 2013 at 1:09 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Mon, Sep 16, 2013 at 9:43 AM, Kees Cook <keescook@chromium.org> wrote:
>> All users of %n are calculating padding size when using seq_file, so
>> instead use the new last_len member for discovering the length of the
>> written strings.
>
> Would it make sense to provide a seq_pad(...) function instead, to avoid
> exposing more seq_file internals to its callers?

We'd still need to track how much to pad.

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH 1/2] remove all uses of printf's %n

[Tetsuo Handa]

Kees Cook wrote:
> On Mon, Sep 16, 2013 at 1:09 AM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > On Mon, Sep 16, 2013 at 9:43 AM, Kees Cook <keescook@chromium.org> wrote:
> >> All users of %n are calculating padding size when using seq_file, so
> >> instead use the new last_len member for discovering the length of the
> >> written strings.
> >
> > Would it make sense to provide a seq_pad(...) function instead, to avoid
> > exposing more seq_file internals to its callers?
> 
> We'd still need to track how much to pad.

If we add "size_t pad_until;" to "struct seq_file", we can do

void seq_setwidth(struct seq_file *m, size_t size)
{
	m->pad_until = m->count + size;
}

void seq_pad(struct seq_file *m, char c)
{
	int size = m->pad_until - m->count;
	if (size > 0)
		seq_printf(m, "%*s", size, "");
	if (c)
		seq_putc(m, c);
}

and use like

  seq_setwidth(m, 21);
  seq_printf(m, "%s%d", con->name, con->index);
  seq_pad(m, '\n');

.

Re: [PATCH 1/2] remove all uses of printf's %n

[Kees Cook]

On Tue, Sep 17, 2013 at 6:06 AM, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
> Kees Cook wrote:
>> On Mon, Sep 16, 2013 at 1:09 AM, Geert Uytterhoeven
>> <geert@linux-m68k.org> wrote:
>> > On Mon, Sep 16, 2013 at 9:43 AM, Kees Cook <keescook@chromium.org> wrote:
>> >> All users of %n are calculating padding size when using seq_file, so
>> >> instead use the new last_len member for discovering the length of the
>> >> written strings.
>> >
>> > Would it make sense to provide a seq_pad(...) function instead, to avoid
>> > exposing more seq_file internals to its callers?
>>
>> We'd still need to track how much to pad.
>
> If we add "size_t pad_until;" to "struct seq_file", we can do
>
> void seq_setwidth(struct seq_file *m, size_t size)
> {
>         m->pad_until = m->count + size;
> }
>
> void seq_pad(struct seq_file *m, char c)
> {
>         int size = m->pad_until - m->count;
>         if (size > 0)
>                 seq_printf(m, "%*s", size, "");
>         if (c)
>                 seq_putc(m, c);
> }
>
> and use like
>
>   seq_setwidth(m, 21);
>   seq_printf(m, "%s%d", con->name, con->index);
>   seq_pad(m, '\n');

Ooh, I like this a lot! Much cleaner.

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH 1/2] remove all uses of printf's %n

[George Spelvin]

>>   seq_setwidth(m, 21);
>>   seq_printf(m, "%s%d", con->name, con->index);
>>   seq_pad(m, '\n');

> Ooh, I like this a lot! Much cleaner.

That's certainly a good way to do it, too.
My "general principles" filter thinks it should be in a local variable
if it can, but if hiding it in the struct seq_file is fine if people
find that cleaner.

seq_pad could also reset the field to some null value and warn if
it's found in that state.  Not a strong preference, though; the error is
fairly harmless and you might not feel it's worth the code size.

Re: [PATCH 1/2] remove all uses of printf's %n

[Tetsuo Handa]

George Spelvin wrote:
> >>   seq_setwidth(m, 21);
> >>   seq_printf(m, "%s%d", con->name, con->index);
> >>   seq_pad(m, '\n');
> 
> > Ooh, I like this a lot! Much cleaner.
> 
> That's certainly a good way to do it, too.
> My "general principles" filter thinks it should be in a local variable
> if it can, but if hiding it in the struct seq_file is fine if people
> find that cleaner.

I think the good point of seq_file is that users don't need to worry about
length calculation for overflow detection. This patch helps users to simplify
length calculation for alignment. I think this approach is better if adding
a size_t to the seq_file is acceptable.

So, the patch follows.
----------------------------------------
>From 02b28fd709971f71e5de9a5b595ff4fd059028b3 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Thu, 19 Sep 2013 17:23:17 +0900
Subject: [PATCH] seq_file: Introduce seq_setwidth() and seq_pad()

There are several users who want to know bytes written by seq_*() for alignment
purpose. Currently they are using %n format for knowing it because seq_*()
returns 0 on success.

This patch introduces seq_setwidth() and seq_pad() for allowing them to align
without using %n format.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 fs/seq_file.c            |   15 +++++++++++++++
 include/linux/seq_file.h |   15 +++++++++++++++
 2 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/fs/seq_file.c b/fs/seq_file.c
index 3135c25..40e471e 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -764,6 +764,21 @@ int seq_write(struct seq_file *seq, const void *data, size_t len)
 }
 EXPORT_SYMBOL(seq_write);
 
+/**
+ * seq_pad - write padding spaces to buffer
+ * @m: seq_file identifying the buffer to which data should be written
+ * @c: the byte to append after padding if non-zero
+ */
+void seq_pad(struct seq_file *m, char c)
+{
+	int size = m->pad_until - m->count;
+	if (size > 0)
+		seq_printf(m, "%*s", size, "");
+	if (c)
+		seq_putc(m, c);
+}
+EXPORT_SYMBOL(seq_pad);
+
 struct list_head *seq_list_start(struct list_head *head, loff_t pos)
 {
 	struct list_head *lh;
diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
index 4e32edc..52e0097 100644
--- a/include/linux/seq_file.h
+++ b/include/linux/seq_file.h
@@ -20,6 +20,7 @@ struct seq_file {
 	size_t size;
 	size_t from;
 	size_t count;
+	size_t pad_until;
 	loff_t index;
 	loff_t read_pos;
 	u64 version;
@@ -79,6 +80,20 @@ static inline void seq_commit(struct seq_file *m, int num)
 	}
 }
 
+/**
+ * seq_setwidth - set padding width
+ * @m: the seq_file handle
+ * @size: the max number of bytes to pad.
+ *
+ * Call seq_setwidth() for setting max width, then call seq_printf() etc. and
+ * finally call seq_pad() to pad the remaining bytes.
+ */
+static inline void seq_setwidth(struct seq_file *m, size_t size)
+{
+	m->pad_until = m->count + size;
+}
+void seq_pad(struct seq_file *m, char c);
+
 char *mangle_path(char *s, const char *p, const char *esc);
 int seq_open(struct file *, const struct seq_operations *);
 ssize_t seq_read(struct file *, char __user *, size_t, loff_t *);
-- 
1.7.1

Re: [PATCH 1/2] remove all uses of printf's %n

[Kees Cook]

On Thu, Sep 19, 2013 at 1:56 AM, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
> George Spelvin wrote:
>> >>   seq_setwidth(m, 21);
>> >>   seq_printf(m, "%s%d", con->name, con->index);
>> >>   seq_pad(m, '\n');
>>
>> > Ooh, I like this a lot! Much cleaner.
>>
>> That's certainly a good way to do it, too.
>> My "general principles" filter thinks it should be in a local variable
>> if it can, but if hiding it in the struct seq_file is fine if people
>> find that cleaner.
>
> I think the good point of seq_file is that users don't need to worry about
> length calculation for overflow detection. This patch helps users to simplify
> length calculation for alignment. I think this approach is better if adding
> a size_t to the seq_file is acceptable.
>
> So, the patch follows.
> ----------------------------------------
> >From 02b28fd709971f71e5de9a5b595ff4fd059028b3 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Thu, 19 Sep 2013 17:23:17 +0900
> Subject: [PATCH] seq_file: Introduce seq_setwidth() and seq_pad()
>
> There are several users who want to know bytes written by seq_*() for alignment
> purpose. Currently they are using %n format for knowing it because seq_*()
> returns 0 on success.
>
> This patch introduces seq_setwidth() and seq_pad() for allowing them to align
> without using %n format.
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

This really feels like a clean solution to me.

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  fs/seq_file.c            |   15 +++++++++++++++
>  include/linux/seq_file.h |   15 +++++++++++++++
>  2 files changed, 30 insertions(+), 0 deletions(-)
>
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index 3135c25..40e471e 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -764,6 +764,21 @@ int seq_write(struct seq_file *seq, const void *data, size_t len)
>  }
>  EXPORT_SYMBOL(seq_write);
>
> +/**
> + * seq_pad - write padding spaces to buffer
> + * @m: seq_file identifying the buffer to which data should be written
> + * @c: the byte to append after padding if non-zero
> + */
> +void seq_pad(struct seq_file *m, char c)
> +{
> +       int size = m->pad_until - m->count;
> +       if (size > 0)
> +               seq_printf(m, "%*s", size, "");
> +       if (c)
> +               seq_putc(m, c);
> +}
> +EXPORT_SYMBOL(seq_pad);
> +
>  struct list_head *seq_list_start(struct list_head *head, loff_t pos)
>  {
>         struct list_head *lh;
> diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
> index 4e32edc..52e0097 100644
> --- a/include/linux/seq_file.h
> +++ b/include/linux/seq_file.h
> @@ -20,6 +20,7 @@ struct seq_file {
>         size_t size;
>         size_t from;
>         size_t count;
> +       size_t pad_until;
>         loff_t index;
>         loff_t read_pos;
>         u64 version;
> @@ -79,6 +80,20 @@ static inline void seq_commit(struct seq_file *m, int num)
>         }
>  }
>
> +/**
> + * seq_setwidth - set padding width
> + * @m: the seq_file handle
> + * @size: the max number of bytes to pad.
> + *
> + * Call seq_setwidth() for setting max width, then call seq_printf() etc. and
> + * finally call seq_pad() to pad the remaining bytes.
> + */
> +static inline void seq_setwidth(struct seq_file *m, size_t size)
> +{
> +       m->pad_until = m->count + size;
> +}
> +void seq_pad(struct seq_file *m, char c);
> +
>  char *mangle_path(char *s, const char *p, const char *esc);
>  int seq_open(struct file *, const struct seq_operations *);
>  ssize_t seq_read(struct file *, char __user *, size_t, loff_t *);
> --
> 1.7.1



-- 
Kees Cook
Chrome OS Security

Re: [PATCH 1/2] remove all uses of printf's %n

[Tetsuo Handa]

Hello.

We are discussing about removal of %n support from vsnprintf() at
https://lkml.org/lkml/2013/9/16/52 , and you are using %n in seq_printf().

I posted https://lkml.org/lkml/diff/2013/9/19/53/1 which introduces
seq_setwidth() / seq_pad() which can avoid use of %n in seq_printf().
Assuming that this patch is merged, would you confirm that I didn't break
your code with below patch?

Regards.
----------------------------------------
>From f8b60ebe3971901b93dedb8eee0f85b60d0fdc5f Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Fri, 20 Sep 2013 12:01:07 +0900
Subject: [PATCH] Remove "%n" usage from seq_file users.

All seq_printf() users are using "%n" for calculating padding size, convert
them to use seq_setwidth() / seq_pad() pair.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 fs/proc/consoles.c   |   10 ++++------
 fs/proc/nommu.c      |   12 +++++-------
 fs/proc/task_mmu.c   |   20 ++++++--------------
 fs/proc/task_nommu.c |   19 ++++++-------------
 net/ipv4/fib_trie.c  |   13 +++++++------
 net/ipv4/ping.c      |   15 +++++++--------
 net/ipv4/tcp_ipv4.c  |   33 +++++++++++++++------------------
 net/ipv4/udp.c       |   15 +++++++--------
 net/phonet/socket.c  |   24 +++++++++++-------------
 net/sctp/objcnt.c    |    9 +++++----
 10 files changed, 73 insertions(+), 97 deletions(-)

diff --git a/fs/proc/consoles.c b/fs/proc/consoles.c
index b701eaa..51942d5 100644
--- a/fs/proc/consoles.c
+++ b/fs/proc/consoles.c
@@ -29,7 +29,6 @@ static int show_console_dev(struct seq_file *m, void *v)
 	char flags[ARRAY_SIZE(con_flags) + 1];
 	struct console *con = v;
 	unsigned int a;
-	int len;
 	dev_t dev = 0;
 
 	if (con->device) {
@@ -47,11 +46,10 @@ static int show_console_dev(struct seq_file *m, void *v)
 			con_flags[a].name : ' ';
 	flags[a] = 0;
 
-	seq_printf(m, "%s%d%n", con->name, con->index, &len);
-	len = 21 - len;
-	if (len < 1)
-		len = 1;
-	seq_printf(m, "%*c%c%c%c (%s)", len, ' ', con->read ? 'R' : '-',
+	seq_setwidth(m, 21 - 1);
+	seq_printf(m, "%s%d", con->name, con->index);
+	seq_pad(m, ' ');
+	seq_printf(m, "%c%c%c (%s)", con->read ? 'R' : '-',
 			con->write ? 'W' : '-', con->unblank ? 'U' : '-',
 			flags);
 	if (dev)
diff --git a/fs/proc/nommu.c b/fs/proc/nommu.c
index ccfd99b..5f9bc8a 100644
--- a/fs/proc/nommu.c
+++ b/fs/proc/nommu.c
@@ -39,7 +39,7 @@ static int nommu_region_show(struct seq_file *m, struct vm_region *region)
 	unsigned long ino = 0;
 	struct file *file;
 	dev_t dev = 0;
-	int flags, len;
+	int flags;
 
 	flags = region->vm_flags;
 	file = region->vm_file;
@@ -50,8 +50,9 @@ static int nommu_region_show(struct seq_file *m, struct vm_region *region)
 		ino = inode->i_ino;
 	}
 
+	seq_setwidth(m, 25 + sizeof(void *) * 6 - 1);
 	seq_printf(m,
-		   "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu %n",
+		   "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu ",
 		   region->vm_start,
 		   region->vm_end,
 		   flags & VM_READ ? 'r' : '-',
@@ -59,13 +60,10 @@ static int nommu_region_show(struct seq_file *m, struct vm_region *region)
 		   flags & VM_EXEC ? 'x' : '-',
 		   flags & VM_MAYSHARE ? flags & VM_SHARED ? 'S' : 's' : 'p',
 		   ((loff_t)region->vm_pgoff) << PAGE_SHIFT,
-		   MAJOR(dev), MINOR(dev), ino, &len);
+		   MAJOR(dev), MINOR(dev), ino);
 
 	if (file) {
-		len = 25 + sizeof(void *) * 6 - len;
-		if (len < 1)
-			len = 1;
-		seq_printf(m, "%*c", len, ' ');
+		seq_pad(m, ' ');
 		seq_path(m, &file->f_path, "");
 	}
 
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 7366e9d..cc24165 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -83,14 +83,6 @@ unsigned long task_statm(struct mm_struct *mm,
 	return mm->total_vm;
 }
 
-static void pad_len_spaces(struct seq_file *m, int len)
-{
-	len = 25 + sizeof(void*) * 6 - len;
-	if (len < 1)
-		len = 1;
-	seq_printf(m, "%*c", len, ' ');
-}
-
 #ifdef CONFIG_NUMA
 /*
  * These functions are for numa_maps but called in generic **maps seq_file
@@ -268,7 +260,6 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma, int is_pid)
 	unsigned long long pgoff = 0;
 	unsigned long start, end;
 	dev_t dev = 0;
-	int len;
 	const char *name = NULL;
 
 	if (file) {
@@ -286,7 +277,8 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma, int is_pid)
 	if (stack_guard_page_end(vma, end))
 		end -= PAGE_SIZE;
 
-	seq_printf(m, "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu %n",
+	seq_setwidth(m, 25 + sizeof(void *) * 6 - 1);
+	seq_printf(m, "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu ",
 			start,
 			end,
 			flags & VM_READ ? 'r' : '-',
@@ -294,14 +286,14 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma, int is_pid)
 			flags & VM_EXEC ? 'x' : '-',
 			flags & VM_MAYSHARE ? 's' : 'p',
 			pgoff,
-			MAJOR(dev), MINOR(dev), ino, &len);
+			MAJOR(dev), MINOR(dev), ino);
 
 	/*
 	 * Print the dentry name for named mappings, and a
 	 * special [heap] marker for the heap:
 	 */
 	if (file) {
-		pad_len_spaces(m, len);
+		seq_pad(m, ' ');
 		seq_path(m, &file->f_path, "\n");
 		goto done;
 	}
@@ -333,7 +325,7 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma, int is_pid)
 				name = "[stack]";
 			} else {
 				/* Thread stack in /proc/PID/maps */
-				pad_len_spaces(m, len);
+				seq_pad(m, ' ');
 				seq_printf(m, "[stack:%d]", tid);
 			}
 		}
@@ -341,7 +333,7 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma, int is_pid)
 
 done:
 	if (name) {
-		pad_len_spaces(m, len);
+		seq_pad(m, ' ');
 		seq_puts(m, name);
 	}
 	seq_putc(m, '\n');
diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
index 56123a6..678455d 100644
--- a/fs/proc/task_nommu.c
+++ b/fs/proc/task_nommu.c
@@ -123,14 +123,6 @@ unsigned long task_statm(struct mm_struct *mm,
 	return size;
 }
 
-static void pad_len_spaces(struct seq_file *m, int len)
-{
-	len = 25 + sizeof(void*) * 6 - len;
-	if (len < 1)
-		len = 1;
-	seq_printf(m, "%*c", len, ' ');
-}
-
 /*
  * display a single VMA to a sequenced file
  */
@@ -142,7 +134,7 @@ static int nommu_vma_show(struct seq_file *m, struct vm_area_struct *vma,
 	unsigned long ino = 0;
 	struct file *file;
 	dev_t dev = 0;
-	int flags, len;
+	int flags;
 	unsigned long long pgoff = 0;
 
 	flags = vma->vm_flags;
@@ -155,8 +147,9 @@ static int nommu_vma_show(struct seq_file *m, struct vm_area_struct *vma,
 		pgoff = (loff_t)vma->vm_pgoff << PAGE_SHIFT;
 	}
 
+	seq_setwidth(m, 25 + sizeof(void *) * 6 - 1);
 	seq_printf(m,
-		   "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu %n",
+		   "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu ",
 		   vma->vm_start,
 		   vma->vm_end,
 		   flags & VM_READ ? 'r' : '-',
@@ -164,16 +157,16 @@ static int nommu_vma_show(struct seq_file *m, struct vm_area_struct *vma,
 		   flags & VM_EXEC ? 'x' : '-',
 		   flags & VM_MAYSHARE ? flags & VM_SHARED ? 'S' : 's' : 'p',
 		   pgoff,
-		   MAJOR(dev), MINOR(dev), ino, &len);
+		   MAJOR(dev), MINOR(dev), ino);
 
 	if (file) {
-		pad_len_spaces(m, len);
+		seq_pad(m, ' ');
 		seq_path(m, &file->f_path, "");
 	} else if (mm) {
 		pid_t tid = vm_is_stack(priv->task, vma, is_pid);
 
 		if (tid != 0) {
-			pad_len_spaces(m, len);
+			seq_pad(m, ' ');
 			/*
 			 * Thread stack in /proc/PID/task/TID/maps or
 			 * the main process stack.
diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
index 3df6d3e..b1af50e 100644
--- a/net/ipv4/fib_trie.c
+++ b/net/ipv4/fib_trie.c
@@ -2530,16 +2530,17 @@ static int fib_route_seq_show(struct seq_file *seq, void *v)
 		list_for_each_entry_rcu(fa, &li->falh, fa_list) {
 			const struct fib_info *fi = fa->fa_info;
 			unsigned int flags = fib_flag_trans(fa->fa_type, mask, fi);
-			int len;
 
 			if (fa->fa_type == RTN_BROADCAST
 			    || fa->fa_type == RTN_MULTICAST)
 				continue;
 
+			seq_setwidth(seq, 127);
+
 			if (fi)
 				seq_printf(seq,
 					 "%s\t%08X\t%08X\t%04X\t%d\t%u\t"
-					 "%d\t%08X\t%d\t%u\t%u%n",
+					 "%d\t%08X\t%d\t%u\t%u",
 					 fi->fib_dev ? fi->fib_dev->name : "*",
 					 prefix,
 					 fi->fib_nh->nh_gw, flags, 0, 0,
@@ -2548,15 +2549,15 @@ static int fib_route_seq_show(struct seq_file *seq, void *v)
 					 (fi->fib_advmss ?
 					  fi->fib_advmss + 40 : 0),
 					 fi->fib_window,
-					 fi->fib_rtt >> 3, &len);
+					 fi->fib_rtt >> 3);
 			else
 				seq_printf(seq,
 					 "*\t%08X\t%08X\t%04X\t%d\t%u\t"
-					 "%d\t%08X\t%d\t%u\t%u%n",
+					 "%d\t%08X\t%d\t%u\t%u",
 					 prefix, 0, flags, 0, 0, 0,
-					 mask, 0, 0, 0, &len);
+					 mask, 0, 0, 0);
 
-			seq_printf(seq, "%*s\n", 127 - len, "");
+			seq_pad(seq, '\n');
 		}
 	}
 
diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index d7d9882..94cc685 100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -1073,7 +1073,7 @@ void ping_seq_stop(struct seq_file *seq, void *v)
 EXPORT_SYMBOL_GPL(ping_seq_stop);
 
 static void ping_v4_format_sock(struct sock *sp, struct seq_file *f,
-		int bucket, int *len)
+		int bucket)
 {
 	struct inet_sock *inet = inet_sk(sp);
 	__be32 dest = inet->inet_daddr;
@@ -1082,7 +1082,7 @@ static void ping_v4_format_sock(struct sock *sp, struct seq_file *f,
 	__u16 srcp = ntohs(inet->inet_sport);
 
 	seq_printf(f, "%5d: %08X:%04X %08X:%04X"
-		" %02X %08X:%08X %02X:%08lX %08X %5u %8d %lu %d %pK %d%n",
+		" %02X %08X:%08X %02X:%08lX %08X %5u %8d %lu %d %pK %d",
 		bucket, src, srcp, dest, destp, sp->sk_state,
 		sk_wmem_alloc_get(sp),
 		sk_rmem_alloc_get(sp),
@@ -1090,23 +1090,22 @@ static void ping_v4_format_sock(struct sock *sp, struct seq_file *f,
 		from_kuid_munged(seq_user_ns(f), sock_i_uid(sp)),
 		0, sock_i_ino(sp),
 		atomic_read(&sp->sk_refcnt), sp,
-		atomic_read(&sp->sk_drops), len);
+		atomic_read(&sp->sk_drops));
 }
 
 static int ping_v4_seq_show(struct seq_file *seq, void *v)
 {
+	seq_setwidth(seq, 127);
 	if (v == SEQ_START_TOKEN)
-		seq_printf(seq, "%-127s\n",
-			   "  sl  local_address rem_address   st tx_queue "
+		seq_puts(seq, "  sl  local_address rem_address   st tx_queue "
 			   "rx_queue tr tm->when retrnsmt   uid  timeout "
 			   "inode ref pointer drops");
 	else {
 		struct ping_iter_state *state = seq->private;
-		int len;
 
-		ping_v4_format_sock(v, seq, state->bucket, &len);
-		seq_printf(seq, "%*s\n", 127 - len, "");
+		ping_v4_format_sock(v, seq, state->bucket);
 	}
+	seq_pad(seq, '\n');
 	return 0;
 }
 
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index b14266b..2948b76 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -2598,13 +2598,13 @@ void tcp_proc_unregister(struct net *net, struct tcp_seq_afinfo *afinfo)
 EXPORT_SYMBOL(tcp_proc_unregister);
 
 static void get_openreq4(const struct sock *sk, const struct request_sock *req,
-			 struct seq_file *f, int i, kuid_t uid, int *len)
+			 struct seq_file *f, int i, kuid_t uid)
 {
 	const struct inet_request_sock *ireq = inet_rsk(req);
 	long delta = req->expires - jiffies;
 
 	seq_printf(f, "%4d: %08X:%04X %08X:%04X"
-		" %02X %08X:%08X %02X:%08lX %08X %5u %8d %u %d %pK%n",
+		" %02X %08X:%08X %02X:%08lX %08X %5u %8d %u %d %pK",
 		i,
 		ireq->loc_addr,
 		ntohs(inet_sk(sk)->inet_sport),
@@ -2619,11 +2619,10 @@ static void get_openreq4(const struct sock *sk, const struct request_sock *req,
 		0,  /* non standard timer */
 		0, /* open_requests have no inode */
 		atomic_read(&sk->sk_refcnt),
-		req,
-		len);
+		req);
 }
 
-static void get_tcp4_sock(struct sock *sk, struct seq_file *f, int i, int *len)
+static void get_tcp4_sock(struct sock *sk, struct seq_file *f, int i)
 {
 	int timer_active;
 	unsigned long timer_expires;
@@ -2662,7 +2661,7 @@ static void get_tcp4_sock(struct sock *sk, struct seq_file *f, int i, int *len)
 		rx_queue = max_t(int, tp->rcv_nxt - tp->copied_seq, 0);
 
 	seq_printf(f, "%4d: %08X:%04X %08X:%04X %02X %08X:%08X %02X:%08lX "
-			"%08X %5u %8d %lu %d %pK %lu %lu %u %u %d%n",
+			"%08X %5u %8d %lu %d %pK %lu %lu %u %u %d",
 		i, src, srcp, dest, destp, sk->sk_state,
 		tp->write_seq - tp->snd_una,
 		rx_queue,
@@ -2679,12 +2678,11 @@ static void get_tcp4_sock(struct sock *sk, struct seq_file *f, int i, int *len)
 		tp->snd_cwnd,
 		sk->sk_state == TCP_LISTEN ?
 		    (fastopenq ? fastopenq->max_qlen : 0) :
-		    (tcp_in_initial_slowstart(tp) ? -1 : tp->snd_ssthresh),
-		len);
+		    (tcp_in_initial_slowstart(tp) ? -1 : tp->snd_ssthresh));
 }
 
 static void get_timewait4_sock(const struct inet_timewait_sock *tw,
-			       struct seq_file *f, int i, int *len)
+			       struct seq_file *f, int i)
 {
 	__be32 dest, src;
 	__u16 destp, srcp;
@@ -2696,10 +2694,10 @@ static void get_timewait4_sock(const struct inet_timewait_sock *tw,
 	srcp  = ntohs(tw->tw_sport);
 
 	seq_printf(f, "%4d: %08X:%04X %08X:%04X"
-		" %02X %08X:%08X %02X:%08lX %08X %5d %8d %d %d %pK%n",
+		" %02X %08X:%08X %02X:%08lX %08X %5d %8d %d %d %pK",
 		i, src, srcp, dest, destp, tw->tw_substate, 0, 0,
 		3, jiffies_delta_to_clock_t(delta), 0, 0, 0, 0,
-		atomic_read(&tw->tw_refcnt), tw, len);
+		atomic_read(&tw->tw_refcnt), tw);
 }
 
 #define TMPSZ 150
@@ -2707,11 +2705,10 @@ static void get_timewait4_sock(const struct inet_timewait_sock *tw,
 static int tcp4_seq_show(struct seq_file *seq, void *v)
 {
 	struct tcp_iter_state *st;
-	int len;
 
+	seq_setwidth(seq, TMPSZ - 1);
 	if (v == SEQ_START_TOKEN) {
-		seq_printf(seq, "%-*s\n", TMPSZ - 1,
-			   "  sl  local_address rem_address   st tx_queue "
+		seq_puts(seq, "  sl  local_address rem_address   st tx_queue "
 			   "rx_queue tr tm->when retrnsmt   uid  timeout "
 			   "inode");
 		goto out;
@@ -2721,17 +2718,17 @@ static int tcp4_seq_show(struct seq_file *seq, void *v)
 	switch (st->state) {
 	case TCP_SEQ_STATE_LISTENING:
 	case TCP_SEQ_STATE_ESTABLISHED:
-		get_tcp4_sock(v, seq, st->num, &len);
+		get_tcp4_sock(v, seq, st->num);
 		break;
 	case TCP_SEQ_STATE_OPENREQ:
-		get_openreq4(st->syn_wait_sk, v, seq, st->num, st->uid, &len);
+		get_openreq4(st->syn_wait_sk, v, seq, st->num, st->uid);
 		break;
 	case TCP_SEQ_STATE_TIME_WAIT:
-		get_timewait4_sock(v, seq, st->num, &len);
+		get_timewait4_sock(v, seq, st->num);
 		break;
 	}
-	seq_printf(seq, "%*s\n", TMPSZ - 1 - len, "");
 out:
+	seq_pad(seq, '\n');
 	return 0;
 }
 
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 74d2c95..31c132c 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -2150,7 +2150,7 @@ EXPORT_SYMBOL(udp_proc_unregister);
 
 /* ------------------------------------------------------------------------ */
 static void udp4_format_sock(struct sock *sp, struct seq_file *f,
-		int bucket, int *len)
+		int bucket)
 {
 	struct inet_sock *inet = inet_sk(sp);
 	__be32 dest = inet->inet_daddr;
@@ -2159,7 +2159,7 @@ static void udp4_format_sock(struct sock *sp, struct seq_file *f,
 	__u16 srcp	  = ntohs(inet->inet_sport);
 
 	seq_printf(f, "%5d: %08X:%04X %08X:%04X"
-		" %02X %08X:%08X %02X:%08lX %08X %5u %8d %lu %d %pK %d%n",
+		" %02X %08X:%08X %02X:%08lX %08X %5u %8d %lu %d %pK %d",
 		bucket, src, srcp, dest, destp, sp->sk_state,
 		sk_wmem_alloc_get(sp),
 		sk_rmem_alloc_get(sp),
@@ -2167,23 +2167,22 @@ static void udp4_format_sock(struct sock *sp, struct seq_file *f,
 		from_kuid_munged(seq_user_ns(f), sock_i_uid(sp)),
 		0, sock_i_ino(sp),
 		atomic_read(&sp->sk_refcnt), sp,
-		atomic_read(&sp->sk_drops), len);
+		atomic_read(&sp->sk_drops));
 }
 
 int udp4_seq_show(struct seq_file *seq, void *v)
 {
+	seq_setwidth(seq, 127);
 	if (v == SEQ_START_TOKEN)
-		seq_printf(seq, "%-127s\n",
-			   "  sl  local_address rem_address   st tx_queue "
+		seq_puts(seq, "  sl  local_address rem_address   st tx_queue "
 			   "rx_queue tr tm->when retrnsmt   uid  timeout "
 			   "inode ref pointer drops");
 	else {
 		struct udp_iter_state *state = seq->private;
-		int len;
 
-		udp4_format_sock(v, seq, state->bucket, &len);
-		seq_printf(seq, "%*s\n", 127 - len, "");
+		udp4_format_sock(v, seq, state->bucket);
 	}
+	seq_pad(seq, '\n');
 	return 0;
 }
 
diff --git a/net/phonet/socket.c b/net/phonet/socket.c
index 77e38f7..008214a 100644
--- a/net/phonet/socket.c
+++ b/net/phonet/socket.c
@@ -595,26 +595,25 @@ static void pn_sock_seq_stop(struct seq_file *seq, void *v)
 
 static int pn_sock_seq_show(struct seq_file *seq, void *v)
 {
-	int len;
-
+	seq_setwidth(seq, 127);
 	if (v == SEQ_START_TOKEN)
-		seq_printf(seq, "%s%n", "pt  loc  rem rs st tx_queue rx_queue "
-			"  uid inode ref pointer drops", &len);
+		seq_puts(seq, "pt  loc  rem rs st tx_queue rx_queue "
+			"  uid inode ref pointer drops");
 	else {
 		struct sock *sk = v;
 		struct pn_sock *pn = pn_sk(sk);
 
 		seq_printf(seq, "%2d %04X:%04X:%02X %02X %08X:%08X %5d %lu "
-			"%d %pK %d%n",
+			"%d %pK %d",
 			sk->sk_protocol, pn->sobject, pn->dobject,
 			pn->resource, sk->sk_state,
 			sk_wmem_alloc_get(sk), sk_rmem_alloc_get(sk),
 			from_kuid_munged(seq_user_ns(seq), sock_i_uid(sk)),
 			sock_i_ino(sk),
 			atomic_read(&sk->sk_refcnt), sk,
-			atomic_read(&sk->sk_drops), &len);
+			atomic_read(&sk->sk_drops));
 	}
-	seq_printf(seq, "%*s\n", 127 - len, "");
+	seq_pad(seq, '\n');
 	return 0;
 }
 
@@ -785,20 +784,19 @@ static void pn_res_seq_stop(struct seq_file *seq, void *v)
 
 static int pn_res_seq_show(struct seq_file *seq, void *v)
 {
-	int len;
-
+	seq_setwidth(seq, 63);
 	if (v == SEQ_START_TOKEN)
-		seq_printf(seq, "%s%n", "rs   uid inode", &len);
+		seq_puts(seq, "rs   uid inode");
 	else {
 		struct sock **psk = v;
 		struct sock *sk = *psk;
 
-		seq_printf(seq, "%02X %5u %lu%n",
+		seq_printf(seq, "%02X %5u %lu",
 			   (int) (psk - pnres.sk),
 			   from_kuid_munged(seq_user_ns(seq), sock_i_uid(sk)),
-			   sock_i_ino(sk), &len);
+			   sock_i_ino(sk));
 	}
-	seq_printf(seq, "%*s\n", 63 - len, "");
+	seq_pad(seq, '\n');
 	return 0;
 }
 
diff --git a/net/sctp/objcnt.c b/net/sctp/objcnt.c
index 5ea573b..647396b 100644
--- a/net/sctp/objcnt.c
+++ b/net/sctp/objcnt.c
@@ -79,12 +79,13 @@ static sctp_dbg_objcnt_entry_t sctp_dbg_objcnt[] = {
  */
 static int sctp_objcnt_seq_show(struct seq_file *seq, void *v)
 {
-	int i, len;
+	int i;
 
 	i = (int)*(loff_t *)v;
-	seq_printf(seq, "%s: %d%n", sctp_dbg_objcnt[i].label,
-				atomic_read(sctp_dbg_objcnt[i].counter), &len);
-	seq_printf(seq, "%*s\n", 127 - len, "");
+	seq_setwidth(seq, 127);
+	seq_printf(seq, "%s: %d", sctp_dbg_objcnt[i].label,
+				atomic_read(sctp_dbg_objcnt[i].counter));
+	seq_pad(seq, '\n');
 	return 0;
 }
 
-- 
1.7.1

Re: [PATCH 1/2] remove all uses of printf's %n

[Joe Perches]

On Fri, 2013-09-20 at 13:09 +0900, Tetsuo Handa wrote:
> Hello.

Tetsuo-san:

> We are discussing about removal of %n support from vsnprintf() at
> https://lkml.org/lkml/2013/9/16/52 , and you are using %n in seq_printf().

Well, I'm not using (mere alcohol isn't using, right?)
but I still have the same question for Al.

Are there any races here?

> I posted https://lkml.org/lkml/diff/2013/9/19/53/1 which introduces
> seq_setwidth() / seq_pad() which can avoid use of %n in seq_printf().

I still think adding last_len, last_rtn
is sensible.

Re: [PATCH 1/2] remove all uses of printf's %n

[Kees Cook]

On Thu, Sep 19, 2013 at 9:23 PM, Joe Perches <joe@perches.com> wrote:
> On Fri, 2013-09-20 at 13:09 +0900, Tetsuo Handa wrote:
>> Hello.
>
> Tetsuo-san:
>
>> We are discussing about removal of %n support from vsnprintf() at
>> https://lkml.org/lkml/2013/9/16/52 , and you are using %n in seq_printf().
>
> Well, I'm not using (mere alcohol isn't using, right?)
> but I still have the same question for Al.
>
> Are there any races here?

All the call sites I examined were linear. FWIW, I didn't see any races.

-Kees

>
>> I posted https://lkml.org/lkml/diff/2013/9/19/53/1 which introduces
>> seq_setwidth() / seq_pad() which can avoid use of %n in seq_printf().
>
> I still think adding last_len, last_rtn
> is sensible.
>
>



-- 
Kees Cook
Chrome OS Security

Re: [PATCH 1/2] remove all uses of printf's %n

[Jiri Slaby]

On 09/20/2013 06:09 AM, Tetsuo Handa wrote:
> --- a/fs/proc/consoles.c
> +++ b/fs/proc/consoles.c
...
> @@ -47,11 +46,10 @@ static int show_console_dev(struct seq_file *m, void *v)
>  			con_flags[a].name : ' ';
>  	flags[a] = 0;
>  
> -	seq_printf(m, "%s%d%n", con->name, con->index, &len);
> -	len = 21 - len;
> -	if (len < 1)
> -		len = 1;
> -	seq_printf(m, "%*c%c%c%c (%s)", len, ' ', con->read ? 'R' : '-',
> +	seq_setwidth(m, 21 - 1);
> +	seq_printf(m, "%s%d", con->name, con->index);
> +	seq_pad(m, ' ');
> +	seq_printf(m, "%c%c%c (%s)", con->read ? 'R' : '-',
>  			con->write ? 'W' : '-', con->unblank ? 'U' : '-',
>  			flags);

Hello, do you really need seq_setwidth? It makes it really ugly...

Or do we need that all? Couldn't we simply have seq_printf_padded? Or
maybe some % modifier in seq_printf to pad the string?

> --- a/net/ipv4/fib_trie.c
> +++ b/net/ipv4/fib_trie.c
...
> @@ -2548,15 +2549,15 @@ static int fib_route_seq_show(struct seq_file *seq, void *v)
>  					 (fi->fib_advmss ?
>  					  fi->fib_advmss + 40 : 0),
>  					 fi->fib_window,
> -					 fi->fib_rtt >> 3, &len);
> +					 fi->fib_rtt >> 3);
>  			else
>  				seq_printf(seq,
>  					 "*\t%08X\t%08X\t%04X\t%d\t%u\t"
> -					 "%d\t%08X\t%d\t%u\t%u%n",
> +					 "%d\t%08X\t%d\t%u\t%u",
>  					 prefix, 0, flags, 0, 0, 0,
> -					 mask, 0, 0, 0, &len);
> +					 mask, 0, 0, 0);
>  
> -			seq_printf(seq, "%*s\n", 127 - len, "");
> +			seq_pad(seq, '\n');

Hmm, seq_pad is unintuitive. I would say it pads the string by '\n'. Of
course it does not, but...

thanks,
-- 
js
suse labs

Re: [PATCH 1/2] remove all uses of printf's %n

[Kees Cook]

On Fri, Sep 20, 2013 at 1:08 AM, Jiri Slaby <jslaby@suse.cz> wrote:
> On 09/20/2013 06:09 AM, Tetsuo Handa wrote:
>> --- a/fs/proc/consoles.c
>> +++ b/fs/proc/consoles.c
> ...
>> @@ -47,11 +46,10 @@ static int show_console_dev(struct seq_file *m, void *v)
>>                       con_flags[a].name : ' ';
>>       flags[a] = 0;
>>
>> -     seq_printf(m, "%s%d%n", con->name, con->index, &len);
>> -     len = 21 - len;
>> -     if (len < 1)
>> -             len = 1;
>> -     seq_printf(m, "%*c%c%c%c (%s)", len, ' ', con->read ? 'R' : '-',
>> +     seq_setwidth(m, 21 - 1);
>> +     seq_printf(m, "%s%d", con->name, con->index);
>> +     seq_pad(m, ' ');
>> +     seq_printf(m, "%c%c%c (%s)", con->read ? 'R' : '-',
>>                       con->write ? 'W' : '-', con->unblank ? 'U' : '-',
>>                       flags);
>
> Hello, do you really need seq_setwidth? It makes it really ugly...

There are a few problems that have been discussed on the various
threads. Namely, we want to minimize the changes to the seq_file
structure and to not add additional work to all the seq_file users
that don't care about padding. If the seq_file calls always track how
far they're written across each call, we add unneeded work to all the
users. To avoid this, we must identify to the seq_file subsystem where
we want to start tracking the length written. To allow this to be
spread across multiple calls (something the %n can't do), we must
record seq->count at some point, and then compare against it at the
point where we want to perform padding.

> Or do we need that all? Couldn't we simply have seq_printf_padded? Or
> maybe some % modifier in seq_printf to pad the string?

Adding a _padding version of things means we'd have to add it to all
seq_* function that print things (like printing paths, etc). Using
this method, the output doesn't matter. We declare the starting point,
output whatever we need, then perform padding, and continue writing.

I think the declaration/output/pad method seems the least invasive to
existing users of padding, and the highest level of flexibility going
forward for future users.

>> --- a/net/ipv4/fib_trie.c
>> +++ b/net/ipv4/fib_trie.c
> ...
>> @@ -2548,15 +2549,15 @@ static int fib_route_seq_show(struct seq_file *seq, void *v)
>>                                        (fi->fib_advmss ?
>>                                         fi->fib_advmss + 40 : 0),
>>                                        fi->fib_window,
>> -                                      fi->fib_rtt >> 3, &len);
>> +                                      fi->fib_rtt >> 3);
>>                       else
>>                               seq_printf(seq,
>>                                        "*\t%08X\t%08X\t%04X\t%d\t%u\t"
>> -                                      "%d\t%08X\t%d\t%u\t%u%n",
>> +                                      "%d\t%08X\t%d\t%u\t%u",
>>                                        prefix, 0, flags, 0, 0, 0,
>> -                                      mask, 0, 0, 0, &len);
>> +                                      mask, 0, 0, 0);
>>
>> -                     seq_printf(seq, "%*s\n", 127 - len, "");
>> +                     seq_pad(seq, '\n');
>
> Hmm, seq_pad is unintuitive. I would say it pads the string by '\n'. Of
> course it does not, but...

I don't think this is a very serious problem. Currently, the padding
character is always ' ' for all existing callers, so it only makes
sense to make the trailing character an argument.

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH 1/2] remove all uses of printf's %n

[Joe Perches]

On Fri, 2013-09-20 at 12:24 -0700, Kees Cook wrote:
> There are a few problems that have been discussed on the various
> threads. Namely, we want to minimize the changes to the seq_file
> structure and to not add additional work to all the seq_file users
> that don't care about padding.

I don't think saving a couple more values to a struct
is that big a deal.

Re: [PATCH 1/2] remove all uses of printf's %n

[Tetsuo Handa]

Kees Cook wrote:
> >> -                     seq_printf(seq, "%*s\n", 127 - len, "");
> >> +                     seq_pad(seq, '\n');
> >
> > Hmm, seq_pad is unintuitive. I would say it pads the string by '\n'. Of
> > course it does not, but...
> 
> I don't think this is a very serious problem. Currently, the padding
> character is always ' ' for all existing callers, so it only makes
> sense to make the trailing character an argument.

If you want, we can rename seq_pad() to seq_pad_and_putc(). Also we can pass
both the padding character (e.g. ' ') and the trailing character (e.g. '\n')
like seq_pad_and_putc((' ' << 8) | '\n'), though I wonder someone wants to
use '\0', '\t', '\n' etc. as the padding character...

Re: [PATCH 1/2] remove all uses of printf's %n

[George Spelvin]

> If you want, we can rename seq_pad() to seq_pad_and_putc(). Also we can pass
> both the padding character (e.g. ' ') and the trailing character (e.g. '\n')
> like seq_pad_and_putc((' ' << 8) | '\n'), though I wonder someone wants to
> use '\0', '\t', '\n' etc. as the padding character...

How about let that complexity wait until it's needed?  It's not like
it's that big a PITA of a patch to write, and there's a significant
chance it will *never* be needed.

Re: [PATCH 1/2] remove all uses of printf's %n

[Geert Uytterhoeven]

On Sat, Sep 21, 2013 at 2:28 AM, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
> Kees Cook wrote:
>> >> -                     seq_printf(seq, "%*s\n", 127 - len, "");
>> >> +                     seq_pad(seq, '\n');
>> >
>> > Hmm, seq_pad is unintuitive. I would say it pads the string by '\n'. Of
>> > course it does not, but...
>>
>> I don't think this is a very serious problem. Currently, the padding
>> character is always ' ' for all existing callers, so it only makes
>> sense to make the trailing character an argument.
>
> If you want, we can rename seq_pad() to seq_pad_and_putc(). Also we can pass
> both the padding character (e.g. ' ') and the trailing character (e.g. '\n')
> like seq_pad_and_putc((' ' << 8) | '\n'), though I wonder someone wants to
> use '\0', '\t', '\n' etc. as the padding character...

Not those special characters. '-' could be useful for tables (doh,
text-mode graphics
log output).

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/2] remove all uses of printf's %n

[Kees Cook]

[-remi (bouncing), +akpm]

On Thu, Sep 19, 2013 at 9:09 PM, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
> Hello.
>
> We are discussing about removal of %n support from vsnprintf() at
> https://lkml.org/lkml/2013/9/16/52 , and you are using %n in seq_printf().
>
> I posted https://lkml.org/lkml/diff/2013/9/19/53/1 which introduces
> seq_setwidth() / seq_pad() which can avoid use of %n in seq_printf().
> Assuming that this patch is merged, would you confirm that I didn't break
> your code with below patch?

As mentioned in the thread, I think we should carry this with the
patch that adds seq_pad and drops %n. It's the cleanest of the
solutions, adds no CPU overhead to other seq_file users, addresses the
basic need (seq_printf padding) while providing expanded functionality
(tracking padding for any seq_file writes, not just seq_printf).

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

>
> Regards.
> ----------------------------------------
> >From f8b60ebe3971901b93dedb8eee0f85b60d0fdc5f Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Fri, 20 Sep 2013 12:01:07 +0900
> Subject: [PATCH] Remove "%n" usage from seq_file users.
>
> All seq_printf() users are using "%n" for calculating padding size, convert
> them to use seq_setwidth() / seq_pad() pair.
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  fs/proc/consoles.c   |   10 ++++------
>  fs/proc/nommu.c      |   12 +++++-------
>  fs/proc/task_mmu.c   |   20 ++++++--------------
>  fs/proc/task_nommu.c |   19 ++++++-------------
>  net/ipv4/fib_trie.c  |   13 +++++++------
>  net/ipv4/ping.c      |   15 +++++++--------
>  net/ipv4/tcp_ipv4.c  |   33 +++++++++++++++------------------
>  net/ipv4/udp.c       |   15 +++++++--------
>  net/phonet/socket.c  |   24 +++++++++++-------------
>  net/sctp/objcnt.c    |    9 +++++----
>  10 files changed, 73 insertions(+), 97 deletions(-)
>
> diff --git a/fs/proc/consoles.c b/fs/proc/consoles.c
> index b701eaa..51942d5 100644
> --- a/fs/proc/consoles.c
> +++ b/fs/proc/consoles.c
> @@ -29,7 +29,6 @@ static int show_console_dev(struct seq_file *m, void *v)
>         char flags[ARRAY_SIZE(con_flags) + 1];
>         struct console *con = v;
>         unsigned int a;
> -       int len;
>         dev_t dev = 0;
>
>         if (con->device) {
> @@ -47,11 +46,10 @@ static int show_console_dev(struct seq_file *m, void *v)
>                         con_flags[a].name : ' ';
>         flags[a] = 0;
>
> -       seq_printf(m, "%s%d%n", con->name, con->index, &len);
> -       len = 21 - len;
> -       if (len < 1)
> -               len = 1;
> -       seq_printf(m, "%*c%c%c%c (%s)", len, ' ', con->read ? 'R' : '-',
> +       seq_setwidth(m, 21 - 1);
> +       seq_printf(m, "%s%d", con->name, con->index);
> +       seq_pad(m, ' ');
> +       seq_printf(m, "%c%c%c (%s)", con->read ? 'R' : '-',
>                         con->write ? 'W' : '-', con->unblank ? 'U' : '-',
>                         flags);
>         if (dev)
> diff --git a/fs/proc/nommu.c b/fs/proc/nommu.c
> index ccfd99b..5f9bc8a 100644
> --- a/fs/proc/nommu.c
> +++ b/fs/proc/nommu.c
> @@ -39,7 +39,7 @@ static int nommu_region_show(struct seq_file *m, struct vm_region *region)
>         unsigned long ino = 0;
>         struct file *file;
>         dev_t dev = 0;
> -       int flags, len;
> +       int flags;
>
>         flags = region->vm_flags;
>         file = region->vm_file;
> @@ -50,8 +50,9 @@ static int nommu_region_show(struct seq_file *m, struct vm_region *region)
>                 ino = inode->i_ino;
>         }
>
> +       seq_setwidth(m, 25 + sizeof(void *) * 6 - 1);
>         seq_printf(m,
> -                  "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu %n",
> +                  "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu ",
>                    region->vm_start,
>                    region->vm_end,
>                    flags & VM_READ ? 'r' : '-',
> @@ -59,13 +60,10 @@ static int nommu_region_show(struct seq_file *m, struct vm_region *region)
>                    flags & VM_EXEC ? 'x' : '-',
>                    flags & VM_MAYSHARE ? flags & VM_SHARED ? 'S' : 's' : 'p',
>                    ((loff_t)region->vm_pgoff) << PAGE_SHIFT,
> -                  MAJOR(dev), MINOR(dev), ino, &len);
> +                  MAJOR(dev), MINOR(dev), ino);
>
>         if (file) {
> -               len = 25 + sizeof(void *) * 6 - len;
> -               if (len < 1)
> -                       len = 1;
> -               seq_printf(m, "%*c", len, ' ');
> +               seq_pad(m, ' ');
>                 seq_path(m, &file->f_path, "");
>         }
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 7366e9d..cc24165 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -83,14 +83,6 @@ unsigned long task_statm(struct mm_struct *mm,
>         return mm->total_vm;
>  }
>
> -static void pad_len_spaces(struct seq_file *m, int len)
> -{
> -       len = 25 + sizeof(void*) * 6 - len;
> -       if (len < 1)
> -               len = 1;
> -       seq_printf(m, "%*c", len, ' ');
> -}
> -
>  #ifdef CONFIG_NUMA
>  /*
>   * These functions are for numa_maps but called in generic **maps seq_file
> @@ -268,7 +260,6 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma, int is_pid)
>         unsigned long long pgoff = 0;
>         unsigned long start, end;
>         dev_t dev = 0;
> -       int len;
>         const char *name = NULL;
>
>         if (file) {
> @@ -286,7 +277,8 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma, int is_pid)
>         if (stack_guard_page_end(vma, end))
>                 end -= PAGE_SIZE;
>
> -       seq_printf(m, "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu %n",
> +       seq_setwidth(m, 25 + sizeof(void *) * 6 - 1);
> +       seq_printf(m, "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu ",
>                         start,
>                         end,
>                         flags & VM_READ ? 'r' : '-',
> @@ -294,14 +286,14 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma, int is_pid)
>                         flags & VM_EXEC ? 'x' : '-',
>                         flags & VM_MAYSHARE ? 's' : 'p',
>                         pgoff,
> -                       MAJOR(dev), MINOR(dev), ino, &len);
> +                       MAJOR(dev), MINOR(dev), ino);
>
>         /*
>          * Print the dentry name for named mappings, and a
>          * special [heap] marker for the heap:
>          */
>         if (file) {
> -               pad_len_spaces(m, len);
> +               seq_pad(m, ' ');
>                 seq_path(m, &file->f_path, "\n");
>                 goto done;
>         }
> @@ -333,7 +325,7 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma, int is_pid)
>                                 name = "[stack]";
>                         } else {
>                                 /* Thread stack in /proc/PID/maps */
> -                               pad_len_spaces(m, len);
> +                               seq_pad(m, ' ');
>                                 seq_printf(m, "[stack:%d]", tid);
>                         }
>                 }
> @@ -341,7 +333,7 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma, int is_pid)
>
>  done:
>         if (name) {
> -               pad_len_spaces(m, len);
> +               seq_pad(m, ' ');
>                 seq_puts(m, name);
>         }
>         seq_putc(m, '\n');
> diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
> index 56123a6..678455d 100644
> --- a/fs/proc/task_nommu.c
> +++ b/fs/proc/task_nommu.c
> @@ -123,14 +123,6 @@ unsigned long task_statm(struct mm_struct *mm,
>         return size;
>  }
>
> -static void pad_len_spaces(struct seq_file *m, int len)
> -{
> -       len = 25 + sizeof(void*) * 6 - len;
> -       if (len < 1)
> -               len = 1;
> -       seq_printf(m, "%*c", len, ' ');
> -}
> -
>  /*
>   * display a single VMA to a sequenced file
>   */
> @@ -142,7 +134,7 @@ static int nommu_vma_show(struct seq_file *m, struct vm_area_struct *vma,
>         unsigned long ino = 0;
>         struct file *file;
>         dev_t dev = 0;
> -       int flags, len;
> +       int flags;
>         unsigned long long pgoff = 0;
>
>         flags = vma->vm_flags;
> @@ -155,8 +147,9 @@ static int nommu_vma_show(struct seq_file *m, struct vm_area_struct *vma,
>                 pgoff = (loff_t)vma->vm_pgoff << PAGE_SHIFT;
>         }
>
> +       seq_setwidth(m, 25 + sizeof(void *) * 6 - 1);
>         seq_printf(m,
> -                  "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu %n",
> +                  "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu ",
>                    vma->vm_start,
>                    vma->vm_end,
>                    flags & VM_READ ? 'r' : '-',
> @@ -164,16 +157,16 @@ static int nommu_vma_show(struct seq_file *m, struct vm_area_struct *vma,
>                    flags & VM_EXEC ? 'x' : '-',
>                    flags & VM_MAYSHARE ? flags & VM_SHARED ? 'S' : 's' : 'p',
>                    pgoff,
> -                  MAJOR(dev), MINOR(dev), ino, &len);
> +                  MAJOR(dev), MINOR(dev), ino);
>
>         if (file) {
> -               pad_len_spaces(m, len);
> +               seq_pad(m, ' ');
>                 seq_path(m, &file->f_path, "");
>         } else if (mm) {
>                 pid_t tid = vm_is_stack(priv->task, vma, is_pid);
>
>                 if (tid != 0) {
> -                       pad_len_spaces(m, len);
> +                       seq_pad(m, ' ');
>                         /*
>                          * Thread stack in /proc/PID/task/TID/maps or
>                          * the main process stack.
> diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
> index 3df6d3e..b1af50e 100644
> --- a/net/ipv4/fib_trie.c
> +++ b/net/ipv4/fib_trie.c
> @@ -2530,16 +2530,17 @@ static int fib_route_seq_show(struct seq_file *seq, void *v)
>                 list_for_each_entry_rcu(fa, &li->falh, fa_list) {
>                         const struct fib_info *fi = fa->fa_info;
>                         unsigned int flags = fib_flag_trans(fa->fa_type, mask, fi);
> -                       int len;
>
>                         if (fa->fa_type == RTN_BROADCAST
>                             || fa->fa_type == RTN_MULTICAST)
>                                 continue;
>
> +                       seq_setwidth(seq, 127);
> +
>                         if (fi)
>                                 seq_printf(seq,
>                                          "%s\t%08X\t%08X\t%04X\t%d\t%u\t"
> -                                        "%d\t%08X\t%d\t%u\t%u%n",
> +                                        "%d\t%08X\t%d\t%u\t%u",
>                                          fi->fib_dev ? fi->fib_dev->name : "*",
>                                          prefix,
>                                          fi->fib_nh->nh_gw, flags, 0, 0,
> @@ -2548,15 +2549,15 @@ static int fib_route_seq_show(struct seq_file *seq, void *v)
>                                          (fi->fib_advmss ?
>                                           fi->fib_advmss + 40 : 0),
>                                          fi->fib_window,
> -                                        fi->fib_rtt >> 3, &len);
> +                                        fi->fib_rtt >> 3);
>                         else
>                                 seq_printf(seq,
>                                          "*\t%08X\t%08X\t%04X\t%d\t%u\t"
> -                                        "%d\t%08X\t%d\t%u\t%u%n",
> +                                        "%d\t%08X\t%d\t%u\t%u",
>                                          prefix, 0, flags, 0, 0, 0,
> -                                        mask, 0, 0, 0, &len);
> +                                        mask, 0, 0, 0);
>
> -                       seq_printf(seq, "%*s\n", 127 - len, "");
> +                       seq_pad(seq, '\n');
>                 }
>         }
>
> diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
> index d7d9882..94cc685 100644
> --- a/net/ipv4/ping.c
> +++ b/net/ipv4/ping.c
> @@ -1073,7 +1073,7 @@ void ping_seq_stop(struct seq_file *seq, void *v)
>  EXPORT_SYMBOL_GPL(ping_seq_stop);
>
>  static void ping_v4_format_sock(struct sock *sp, struct seq_file *f,
> -               int bucket, int *len)
> +               int bucket)
>  {
>         struct inet_sock *inet = inet_sk(sp);
>         __be32 dest = inet->inet_daddr;
> @@ -1082,7 +1082,7 @@ static void ping_v4_format_sock(struct sock *sp, struct seq_file *f,
>         __u16 srcp = ntohs(inet->inet_sport);
>
>         seq_printf(f, "%5d: %08X:%04X %08X:%04X"
> -               " %02X %08X:%08X %02X:%08lX %08X %5u %8d %lu %d %pK %d%n",
> +               " %02X %08X:%08X %02X:%08lX %08X %5u %8d %lu %d %pK %d",
>                 bucket, src, srcp, dest, destp, sp->sk_state,
>                 sk_wmem_alloc_get(sp),
>                 sk_rmem_alloc_get(sp),
> @@ -1090,23 +1090,22 @@ static void ping_v4_format_sock(struct sock *sp, struct seq_file *f,
>                 from_kuid_munged(seq_user_ns(f), sock_i_uid(sp)),
>                 0, sock_i_ino(sp),
>                 atomic_read(&sp->sk_refcnt), sp,
> -               atomic_read(&sp->sk_drops), len);
> +               atomic_read(&sp->sk_drops));
>  }
>
>  static int ping_v4_seq_show(struct seq_file *seq, void *v)
>  {
> +       seq_setwidth(seq, 127);
>         if (v == SEQ_START_TOKEN)
> -               seq_printf(seq, "%-127s\n",
> -                          "  sl  local_address rem_address   st tx_queue "
> +               seq_puts(seq, "  sl  local_address rem_address   st tx_queue "
>                            "rx_queue tr tm->when retrnsmt   uid  timeout "
>                            "inode ref pointer drops");
>         else {
>                 struct ping_iter_state *state = seq->private;
> -               int len;
>
> -               ping_v4_format_sock(v, seq, state->bucket, &len);
> -               seq_printf(seq, "%*s\n", 127 - len, "");
> +               ping_v4_format_sock(v, seq, state->bucket);
>         }
> +       seq_pad(seq, '\n');
>         return 0;
>  }
>
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index b14266b..2948b76 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -2598,13 +2598,13 @@ void tcp_proc_unregister(struct net *net, struct tcp_seq_afinfo *afinfo)
>  EXPORT_SYMBOL(tcp_proc_unregister);
>
>  static void get_openreq4(const struct sock *sk, const struct request_sock *req,
> -                        struct seq_file *f, int i, kuid_t uid, int *len)
> +                        struct seq_file *f, int i, kuid_t uid)
>  {
>         const struct inet_request_sock *ireq = inet_rsk(req);
>         long delta = req->expires - jiffies;
>
>         seq_printf(f, "%4d: %08X:%04X %08X:%04X"
> -               " %02X %08X:%08X %02X:%08lX %08X %5u %8d %u %d %pK%n",
> +               " %02X %08X:%08X %02X:%08lX %08X %5u %8d %u %d %pK",
>                 i,
>                 ireq->loc_addr,
>                 ntohs(inet_sk(sk)->inet_sport),
> @@ -2619,11 +2619,10 @@ static void get_openreq4(const struct sock *sk, const struct request_sock *req,
>                 0,  /* non standard timer */
>                 0, /* open_requests have no inode */
>                 atomic_read(&sk->sk_refcnt),
> -               req,
> -               len);
> +               req);
>  }
>
> -static void get_tcp4_sock(struct sock *sk, struct seq_file *f, int i, int *len)
> +static void get_tcp4_sock(struct sock *sk, struct seq_file *f, int i)
>  {
>         int timer_active;
>         unsigned long timer_expires;
> @@ -2662,7 +2661,7 @@ static void get_tcp4_sock(struct sock *sk, struct seq_file *f, int i, int *len)
>                 rx_queue = max_t(int, tp->rcv_nxt - tp->copied_seq, 0);
>
>         seq_printf(f, "%4d: %08X:%04X %08X:%04X %02X %08X:%08X %02X:%08lX "
> -                       "%08X %5u %8d %lu %d %pK %lu %lu %u %u %d%n",
> +                       "%08X %5u %8d %lu %d %pK %lu %lu %u %u %d",
>                 i, src, srcp, dest, destp, sk->sk_state,
>                 tp->write_seq - tp->snd_una,
>                 rx_queue,
> @@ -2679,12 +2678,11 @@ static void get_tcp4_sock(struct sock *sk, struct seq_file *f, int i, int *len)
>                 tp->snd_cwnd,
>                 sk->sk_state == TCP_LISTEN ?
>                     (fastopenq ? fastopenq->max_qlen : 0) :
> -                   (tcp_in_initial_slowstart(tp) ? -1 : tp->snd_ssthresh),
> -               len);
> +                   (tcp_in_initial_slowstart(tp) ? -1 : tp->snd_ssthresh));
>  }
>
>  static void get_timewait4_sock(const struct inet_timewait_sock *tw,
> -                              struct seq_file *f, int i, int *len)
> +                              struct seq_file *f, int i)
>  {
>         __be32 dest, src;
>         __u16 destp, srcp;
> @@ -2696,10 +2694,10 @@ static void get_timewait4_sock(const struct inet_timewait_sock *tw,
>         srcp  = ntohs(tw->tw_sport);
>
>         seq_printf(f, "%4d: %08X:%04X %08X:%04X"
> -               " %02X %08X:%08X %02X:%08lX %08X %5d %8d %d %d %pK%n",
> +               " %02X %08X:%08X %02X:%08lX %08X %5d %8d %d %d %pK",
>                 i, src, srcp, dest, destp, tw->tw_substate, 0, 0,
>                 3, jiffies_delta_to_clock_t(delta), 0, 0, 0, 0,
> -               atomic_read(&tw->tw_refcnt), tw, len);
> +               atomic_read(&tw->tw_refcnt), tw);
>  }
>
>  #define TMPSZ 150
> @@ -2707,11 +2705,10 @@ static void get_timewait4_sock(const struct inet_timewait_sock *tw,
>  static int tcp4_seq_show(struct seq_file *seq, void *v)
>  {
>         struct tcp_iter_state *st;
> -       int len;
>
> +       seq_setwidth(seq, TMPSZ - 1);
>         if (v == SEQ_START_TOKEN) {
> -               seq_printf(seq, "%-*s\n", TMPSZ - 1,
> -                          "  sl  local_address rem_address   st tx_queue "
> +               seq_puts(seq, "  sl  local_address rem_address   st tx_queue "
>                            "rx_queue tr tm->when retrnsmt   uid  timeout "
>                            "inode");
>                 goto out;
> @@ -2721,17 +2718,17 @@ static int tcp4_seq_show(struct seq_file *seq, void *v)
>         switch (st->state) {
>         case TCP_SEQ_STATE_LISTENING:
>         case TCP_SEQ_STATE_ESTABLISHED:
> -               get_tcp4_sock(v, seq, st->num, &len);
> +               get_tcp4_sock(v, seq, st->num);
>                 break;
>         case TCP_SEQ_STATE_OPENREQ:
> -               get_openreq4(st->syn_wait_sk, v, seq, st->num, st->uid, &len);
> +               get_openreq4(st->syn_wait_sk, v, seq, st->num, st->uid);
>                 break;
>         case TCP_SEQ_STATE_TIME_WAIT:
> -               get_timewait4_sock(v, seq, st->num, &len);
> +               get_timewait4_sock(v, seq, st->num);
>                 break;
>         }
> -       seq_printf(seq, "%*s\n", TMPSZ - 1 - len, "");
>  out:
> +       seq_pad(seq, '\n');
>         return 0;
>  }
>
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 74d2c95..31c132c 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -2150,7 +2150,7 @@ EXPORT_SYMBOL(udp_proc_unregister);
>
>  /* ------------------------------------------------------------------------ */
>  static void udp4_format_sock(struct sock *sp, struct seq_file *f,
> -               int bucket, int *len)
> +               int bucket)
>  {
>         struct inet_sock *inet = inet_sk(sp);
>         __be32 dest = inet->inet_daddr;
> @@ -2159,7 +2159,7 @@ static void udp4_format_sock(struct sock *sp, struct seq_file *f,
>         __u16 srcp        = ntohs(inet->inet_sport);
>
>         seq_printf(f, "%5d: %08X:%04X %08X:%04X"
> -               " %02X %08X:%08X %02X:%08lX %08X %5u %8d %lu %d %pK %d%n",
> +               " %02X %08X:%08X %02X:%08lX %08X %5u %8d %lu %d %pK %d",
>                 bucket, src, srcp, dest, destp, sp->sk_state,
>                 sk_wmem_alloc_get(sp),
>                 sk_rmem_alloc_get(sp),
> @@ -2167,23 +2167,22 @@ static void udp4_format_sock(struct sock *sp, struct seq_file *f,
>                 from_kuid_munged(seq_user_ns(f), sock_i_uid(sp)),
>                 0, sock_i_ino(sp),
>                 atomic_read(&sp->sk_refcnt), sp,
> -               atomic_read(&sp->sk_drops), len);
> +               atomic_read(&sp->sk_drops));
>  }
>
>  int udp4_seq_show(struct seq_file *seq, void *v)
>  {
> +       seq_setwidth(seq, 127);
>         if (v == SEQ_START_TOKEN)
> -               seq_printf(seq, "%-127s\n",
> -                          "  sl  local_address rem_address   st tx_queue "
> +               seq_puts(seq, "  sl  local_address rem_address   st tx_queue "
>                            "rx_queue tr tm->when retrnsmt   uid  timeout "
>                            "inode ref pointer drops");
>         else {
>                 struct udp_iter_state *state = seq->private;
> -               int len;
>
> -               udp4_format_sock(v, seq, state->bucket, &len);
> -               seq_printf(seq, "%*s\n", 127 - len, "");
> +               udp4_format_sock(v, seq, state->bucket);
>         }
> +       seq_pad(seq, '\n');
>         return 0;
>  }
>
> diff --git a/net/phonet/socket.c b/net/phonet/socket.c
> index 77e38f7..008214a 100644
> --- a/net/phonet/socket.c
> +++ b/net/phonet/socket.c
> @@ -595,26 +595,25 @@ static void pn_sock_seq_stop(struct seq_file *seq, void *v)
>
>  static int pn_sock_seq_show(struct seq_file *seq, void *v)
>  {
> -       int len;
> -
> +       seq_setwidth(seq, 127);
>         if (v == SEQ_START_TOKEN)
> -               seq_printf(seq, "%s%n", "pt  loc  rem rs st tx_queue rx_queue "
> -                       "  uid inode ref pointer drops", &len);
> +               seq_puts(seq, "pt  loc  rem rs st tx_queue rx_queue "
> +                       "  uid inode ref pointer drops");
>         else {
>                 struct sock *sk = v;
>                 struct pn_sock *pn = pn_sk(sk);
>
>                 seq_printf(seq, "%2d %04X:%04X:%02X %02X %08X:%08X %5d %lu "
> -                       "%d %pK %d%n",
> +                       "%d %pK %d",
>                         sk->sk_protocol, pn->sobject, pn->dobject,
>                         pn->resource, sk->sk_state,
>                         sk_wmem_alloc_get(sk), sk_rmem_alloc_get(sk),
>                         from_kuid_munged(seq_user_ns(seq), sock_i_uid(sk)),
>                         sock_i_ino(sk),
>                         atomic_read(&sk->sk_refcnt), sk,
> -                       atomic_read(&sk->sk_drops), &len);
> +                       atomic_read(&sk->sk_drops));
>         }
> -       seq_printf(seq, "%*s\n", 127 - len, "");
> +       seq_pad(seq, '\n');
>         return 0;
>  }
>
> @@ -785,20 +784,19 @@ static void pn_res_seq_stop(struct seq_file *seq, void *v)
>
>  static int pn_res_seq_show(struct seq_file *seq, void *v)
>  {
> -       int len;
> -
> +       seq_setwidth(seq, 63);
>         if (v == SEQ_START_TOKEN)
> -               seq_printf(seq, "%s%n", "rs   uid inode", &len);
> +               seq_puts(seq, "rs   uid inode");
>         else {
>                 struct sock **psk = v;
>                 struct sock *sk = *psk;
>
> -               seq_printf(seq, "%02X %5u %lu%n",
> +               seq_printf(seq, "%02X %5u %lu",
>                            (int) (psk - pnres.sk),
>                            from_kuid_munged(seq_user_ns(seq), sock_i_uid(sk)),
> -                          sock_i_ino(sk), &len);
> +                          sock_i_ino(sk));
>         }
> -       seq_printf(seq, "%*s\n", 63 - len, "");
> +       seq_pad(seq, '\n');
>         return 0;
>  }
>
> diff --git a/net/sctp/objcnt.c b/net/sctp/objcnt.c
> index 5ea573b..647396b 100644
> --- a/net/sctp/objcnt.c
> +++ b/net/sctp/objcnt.c
> @@ -79,12 +79,13 @@ static sctp_dbg_objcnt_entry_t sctp_dbg_objcnt[] = {
>   */
>  static int sctp_objcnt_seq_show(struct seq_file *seq, void *v)
>  {
> -       int i, len;
> +       int i;
>
>         i = (int)*(loff_t *)v;
> -       seq_printf(seq, "%s: %d%n", sctp_dbg_objcnt[i].label,
> -                               atomic_read(sctp_dbg_objcnt[i].counter), &len);
> -       seq_printf(seq, "%*s\n", 127 - len, "");
> +       seq_setwidth(seq, 127);
> +       seq_printf(seq, "%s: %d", sctp_dbg_objcnt[i].label,
> +                               atomic_read(sctp_dbg_objcnt[i].counter));
> +       seq_pad(seq, '\n');
>         return 0;
>  }
>
> --
> 1.7.1



-- 
Kees Cook
Chrome OS Security

Re: [PATCH 1/2] remove all uses of printf's %n

[Tetsuo Handa]

Hello.

As it seems that there is no critical problem (naming preference can easily be
fixed if needed), can these patches go to linux-next?

If these patches are accepted, Kees Cook will submit a patch which removes %n
support from vsnprintf() ( https://lkml.org/lkml/2013/9/16/54 ).

Regards.
----------------------------------------
>From 02b28fd709971f71e5de9a5b595ff4fd059028b3 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Thu, 19 Sep 2013 17:23:17 +0900
Subject: [PATCH] seq_file: Introduce seq_setwidth() and seq_pad()

There are several users who want to know bytes written by seq_*() for alignment
purpose. Currently they are using %n format for knowing it because seq_*()
returns 0 on success.

This patch introduces seq_setwidth() and seq_pad() for allowing them to align
without using %n format.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Acked-by: Kees Cook <keescook@chromium.org>
---
 fs/seq_file.c            |   15 +++++++++++++++
 include/linux/seq_file.h |   15 +++++++++++++++
 2 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/fs/seq_file.c b/fs/seq_file.c
index 3135c25..40e471e 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -764,6 +764,21 @@ int seq_write(struct seq_file *seq, const void *data, size_t len)
 }
 EXPORT_SYMBOL(seq_write);
 
+/**
+ * seq_pad - write padding spaces to buffer
+ * @m: seq_file identifying the buffer to which data should be written
+ * @c: the byte to append after padding if non-zero
+ */
+void seq_pad(struct seq_file *m, char c)
+{
+	int size = m->pad_until - m->count;
+	if (size > 0)
+		seq_printf(m, "%*s", size, "");
+	if (c)
+		seq_putc(m, c);
+}
+EXPORT_SYMBOL(seq_pad);
+
 struct list_head *seq_list_start(struct list_head *head, loff_t pos)
 {
 	struct list_head *lh;
diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
index 4e32edc..52e0097 100644
--- a/include/linux/seq_file.h
+++ b/include/linux/seq_file.h
@@ -20,6 +20,7 @@ struct seq_file {
 	size_t size;
 	size_t from;
 	size_t count;
+	size_t pad_until;
 	loff_t index;
 	loff_t read_pos;
 	u64 version;
@@ -79,6 +80,20 @@ static inline void seq_commit(struct seq_file *m, int num)
 	}
 }
 
+/**
+ * seq_setwidth - set padding width
+ * @m: the seq_file handle
+ * @size: the max number of bytes to pad.
+ *
+ * Call seq_setwidth() for setting max width, then call seq_printf() etc. and
+ * finally call seq_pad() to pad the remaining bytes.
+ */
+static inline void seq_setwidth(struct seq_file *m, size_t size)
+{
+	m->pad_until = m->count + size;
+}
+void seq_pad(struct seq_file *m, char c);
+
 char *mangle_path(char *s, const char *p, const char *esc);
 int seq_open(struct file *, const struct seq_operations *);
 ssize_t seq_read(struct file *, char __user *, size_t, loff_t *);
-- 
1.7.1
----------------------------------------
>From f8b60ebe3971901b93dedb8eee0f85b60d0fdc5f Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Fri, 20 Sep 2013 12:01:07 +0900
Subject: [PATCH] Remove "%n" usage from seq_file users.

All seq_printf() users are using "%n" for calculating padding size, convert
them to use seq_setwidth() / seq_pad() pair.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Acked-by: Kees Cook <keescook@chromium.org>
---
 fs/proc/consoles.c   |   10 ++++------
 fs/proc/nommu.c      |   12 +++++-------
 fs/proc/task_mmu.c   |   20 ++++++--------------
 fs/proc/task_nommu.c |   19 ++++++-------------
 net/ipv4/fib_trie.c  |   13 +++++++------
 net/ipv4/ping.c      |   15 +++++++--------
 net/ipv4/tcp_ipv4.c  |   33 +++++++++++++++------------------
 net/ipv4/udp.c       |   15 +++++++--------
 net/phonet/socket.c  |   24 +++++++++++-------------
 net/sctp/objcnt.c    |    9 +++++----
 10 files changed, 73 insertions(+), 97 deletions(-)

diff --git a/fs/proc/consoles.c b/fs/proc/consoles.c
index b701eaa..51942d5 100644
--- a/fs/proc/consoles.c
+++ b/fs/proc/consoles.c
@@ -29,7 +29,6 @@ static int show_console_dev(struct seq_file *m, void *v)
 	char flags[ARRAY_SIZE(con_flags) + 1];
 	struct console *con = v;
 	unsigned int a;
-	int len;
 	dev_t dev = 0;
 
 	if (con->device) {
@@ -47,11 +46,10 @@ static int show_console_dev(struct seq_file *m, void *v)
 			con_flags[a].name : ' ';
 	flags[a] = 0;
 
-	seq_printf(m, "%s%d%n", con->name, con->index, &len);
-	len = 21 - len;
-	if (len < 1)
-		len = 1;
-	seq_printf(m, "%*c%c%c%c (%s)", len, ' ', con->read ? 'R' : '-',
+	seq_setwidth(m, 21 - 1);
+	seq_printf(m, "%s%d", con->name, con->index);
+	seq_pad(m, ' ');
+	seq_printf(m, "%c%c%c (%s)", con->read ? 'R' : '-',
 			con->write ? 'W' : '-', con->unblank ? 'U' : '-',
 			flags);
 	if (dev)
diff --git a/fs/proc/nommu.c b/fs/proc/nommu.c
index ccfd99b..5f9bc8a 100644
--- a/fs/proc/nommu.c
+++ b/fs/proc/nommu.c
@@ -39,7 +39,7 @@ static int nommu_region_show(struct seq_file *m, struct vm_region *region)
 	unsigned long ino = 0;
 	struct file *file;
 	dev_t dev = 0;
-	int flags, len;
+	int flags;
 
 	flags = region->vm_flags;
 	file = region->vm_file;
@@ -50,8 +50,9 @@ static int nommu_region_show(struct seq_file *m, struct vm_region *region)
 		ino = inode->i_ino;
 	}
 
+	seq_setwidth(m, 25 + sizeof(void *) * 6 - 1);
 	seq_printf(m,
-		   "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu %n",
+		   "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu ",
 		   region->vm_start,
 		   region->vm_end,
 		   flags & VM_READ ? 'r' : '-',
@@ -59,13 +60,10 @@ static int nommu_region_show(struct seq_file *m, struct vm_region *region)
 		   flags & VM_EXEC ? 'x' : '-',
 		   flags & VM_MAYSHARE ? flags & VM_SHARED ? 'S' : 's' : 'p',
 		   ((loff_t)region->vm_pgoff) << PAGE_SHIFT,
-		   MAJOR(dev), MINOR(dev), ino, &len);
+		   MAJOR(dev), MINOR(dev), ino);
 
 	if (file) {
-		len = 25 + sizeof(void *) * 6 - len;
-		if (len < 1)
-			len = 1;
-		seq_printf(m, "%*c", len, ' ');
+		seq_pad(m, ' ');
 		seq_path(m, &file->f_path, "");
 	}
 
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 7366e9d..cc24165 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -83,14 +83,6 @@ unsigned long task_statm(struct mm_struct *mm,
 	return mm->total_vm;
 }
 
-static void pad_len_spaces(struct seq_file *m, int len)
-{
-	len = 25 + sizeof(void*) * 6 - len;
-	if (len < 1)
-		len = 1;
-	seq_printf(m, "%*c", len, ' ');
-}
-
 #ifdef CONFIG_NUMA
 /*
  * These functions are for numa_maps but called in generic **maps seq_file
@@ -268,7 +260,6 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma, int is_pid)
 	unsigned long long pgoff = 0;
 	unsigned long start, end;
 	dev_t dev = 0;
-	int len;
 	const char *name = NULL;
 
 	if (file) {
@@ -286,7 +277,8 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma, int is_pid)
 	if (stack_guard_page_end(vma, end))
 		end -= PAGE_SIZE;
 
-	seq_printf(m, "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu %n",
+	seq_setwidth(m, 25 + sizeof(void *) * 6 - 1);
+	seq_printf(m, "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu ",
 			start,
 			end,
 			flags & VM_READ ? 'r' : '-',
@@ -294,14 +286,14 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma, int is_pid)
 			flags & VM_EXEC ? 'x' : '-',
 			flags & VM_MAYSHARE ? 's' : 'p',
 			pgoff,
-			MAJOR(dev), MINOR(dev), ino, &len);
+			MAJOR(dev), MINOR(dev), ino);
 
 	/*
 	 * Print the dentry name for named mappings, and a
 	 * special [heap] marker for the heap:
 	 */
 	if (file) {
-		pad_len_spaces(m, len);
+		seq_pad(m, ' ');
 		seq_path(m, &file->f_path, "\n");
 		goto done;
 	}
@@ -333,7 +325,7 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma, int is_pid)
 				name = "[stack]";
 			} else {
 				/* Thread stack in /proc/PID/maps */
-				pad_len_spaces(m, len);
+				seq_pad(m, ' ');
 				seq_printf(m, "[stack:%d]", tid);
 			}
 		}
@@ -341,7 +333,7 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma, int is_pid)
 
 done:
 	if (name) {
-		pad_len_spaces(m, len);
+		seq_pad(m, ' ');
 		seq_puts(m, name);
 	}
 	seq_putc(m, '\n');
diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
index 56123a6..678455d 100644
--- a/fs/proc/task_nommu.c
+++ b/fs/proc/task_nommu.c
@@ -123,14 +123,6 @@ unsigned long task_statm(struct mm_struct *mm,
 	return size;
 }
 
-static void pad_len_spaces(struct seq_file *m, int len)
-{
-	len = 25 + sizeof(void*) * 6 - len;
-	if (len < 1)
-		len = 1;
-	seq_printf(m, "%*c", len, ' ');
-}
-
 /*
  * display a single VMA to a sequenced file
  */
@@ -142,7 +134,7 @@ static int nommu_vma_show(struct seq_file *m, struct vm_area_struct *vma,
 	unsigned long ino = 0;
 	struct file *file;
 	dev_t dev = 0;
-	int flags, len;
+	int flags;
 	unsigned long long pgoff = 0;
 
 	flags = vma->vm_flags;
@@ -155,8 +147,9 @@ static int nommu_vma_show(struct seq_file *m, struct vm_area_struct *vma,
 		pgoff = (loff_t)vma->vm_pgoff << PAGE_SHIFT;
 	}
 
+	seq_setwidth(m, 25 + sizeof(void *) * 6 - 1);
 	seq_printf(m,
-		   "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu %n",
+		   "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu ",
 		   vma->vm_start,
 		   vma->vm_end,
 		   flags & VM_READ ? 'r' : '-',
@@ -164,16 +157,16 @@ static int nommu_vma_show(struct seq_file *m, struct vm_area_struct *vma,
 		   flags & VM_EXEC ? 'x' : '-',
 		   flags & VM_MAYSHARE ? flags & VM_SHARED ? 'S' : 's' : 'p',
 		   pgoff,
-		   MAJOR(dev), MINOR(dev), ino, &len);
+		   MAJOR(dev), MINOR(dev), ino);
 
 	if (file) {
-		pad_len_spaces(m, len);
+		seq_pad(m, ' ');
 		seq_path(m, &file->f_path, "");
 	} else if (mm) {
 		pid_t tid = vm_is_stack(priv->task, vma, is_pid);
 
 		if (tid != 0) {
-			pad_len_spaces(m, len);
+			seq_pad(m, ' ');
 			/*
 			 * Thread stack in /proc/PID/task/TID/maps or
 			 * the main process stack.
diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
index 3df6d3e..b1af50e 100644
--- a/net/ipv4/fib_trie.c
+++ b/net/ipv4/fib_trie.c
@@ -2530,16 +2530,17 @@ static int fib_route_seq_show(struct seq_file *seq, void *v)
 		list_for_each_entry_rcu(fa, &li->falh, fa_list) {
 			const struct fib_info *fi = fa->fa_info;
 			unsigned int flags = fib_flag_trans(fa->fa_type, mask, fi);
-			int len;
 
 			if (fa->fa_type == RTN_BROADCAST
 			    || fa->fa_type == RTN_MULTICAST)
 				continue;
 
+			seq_setwidth(seq, 127);
+
 			if (fi)
 				seq_printf(seq,
 					 "%s\t%08X\t%08X\t%04X\t%d\t%u\t"
-					 "%d\t%08X\t%d\t%u\t%u%n",
+					 "%d\t%08X\t%d\t%u\t%u",
 					 fi->fib_dev ? fi->fib_dev->name : "*",
 					 prefix,
 					 fi->fib_nh->nh_gw, flags, 0, 0,
@@ -2548,15 +2549,15 @@ static int fib_route_seq_show(struct seq_file *seq, void *v)
 					 (fi->fib_advmss ?
 					  fi->fib_advmss + 40 : 0),
 					 fi->fib_window,
-					 fi->fib_rtt >> 3, &len);
+					 fi->fib_rtt >> 3);
 			else
 				seq_printf(seq,
 					 "*\t%08X\t%08X\t%04X\t%d\t%u\t"
-					 "%d\t%08X\t%d\t%u\t%u%n",
+					 "%d\t%08X\t%d\t%u\t%u",
 					 prefix, 0, flags, 0, 0, 0,
-					 mask, 0, 0, 0, &len);
+					 mask, 0, 0, 0);
 
-			seq_printf(seq, "%*s\n", 127 - len, "");
+			seq_pad(seq, '\n');
 		}
 	}
 
diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index d7d9882..94cc685 100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -1073,7 +1073,7 @@ void ping_seq_stop(struct seq_file *seq, void *v)
 EXPORT_SYMBOL_GPL(ping_seq_stop);
 
 static void ping_v4_format_sock(struct sock *sp, struct seq_file *f,
-		int bucket, int *len)
+		int bucket)
 {
 	struct inet_sock *inet = inet_sk(sp);
 	__be32 dest = inet->inet_daddr;
@@ -1082,7 +1082,7 @@ static void ping_v4_format_sock(struct sock *sp, struct seq_file *f,
 	__u16 srcp = ntohs(inet->inet_sport);
 
 	seq_printf(f, "%5d: %08X:%04X %08X:%04X"
-		" %02X %08X:%08X %02X:%08lX %08X %5u %8d %lu %d %pK %d%n",
+		" %02X %08X:%08X %02X:%08lX %08X %5u %8d %lu %d %pK %d",
 		bucket, src, srcp, dest, destp, sp->sk_state,
 		sk_wmem_alloc_get(sp),
 		sk_rmem_alloc_get(sp),
@@ -1090,23 +1090,22 @@ static void ping_v4_format_sock(struct sock *sp, struct seq_file *f,
 		from_kuid_munged(seq_user_ns(f), sock_i_uid(sp)),
 		0, sock_i_ino(sp),
 		atomic_read(&sp->sk_refcnt), sp,
-		atomic_read(&sp->sk_drops), len);
+		atomic_read(&sp->sk_drops));
 }
 
 static int ping_v4_seq_show(struct seq_file *seq, void *v)
 {
+	seq_setwidth(seq, 127);
 	if (v == SEQ_START_TOKEN)
-		seq_printf(seq, "%-127s\n",
-			   "  sl  local_address rem_address   st tx_queue "
+		seq_puts(seq, "  sl  local_address rem_address   st tx_queue "
 			   "rx_queue tr tm->when retrnsmt   uid  timeout "
 			   "inode ref pointer drops");
 	else {
 		struct ping_iter_state *state = seq->private;
-		int len;
 
-		ping_v4_format_sock(v, seq, state->bucket, &len);
-		seq_printf(seq, "%*s\n", 127 - len, "");
+		ping_v4_format_sock(v, seq, state->bucket);
 	}
+	seq_pad(seq, '\n');
 	return 0;
 }
 
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index b14266b..2948b76 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -2598,13 +2598,13 @@ void tcp_proc_unregister(struct net *net, struct tcp_seq_afinfo *afinfo)
 EXPORT_SYMBOL(tcp_proc_unregister);
 
 static void get_openreq4(const struct sock *sk, const struct request_sock *req,
-			 struct seq_file *f, int i, kuid_t uid, int *len)
+			 struct seq_file *f, int i, kuid_t uid)
 {
 	const struct inet_request_sock *ireq = inet_rsk(req);
 	long delta = req->expires - jiffies;
 
 	seq_printf(f, "%4d: %08X:%04X %08X:%04X"
-		" %02X %08X:%08X %02X:%08lX %08X %5u %8d %u %d %pK%n",
+		" %02X %08X:%08X %02X:%08lX %08X %5u %8d %u %d %pK",
 		i,
 		ireq->loc_addr,
 		ntohs(inet_sk(sk)->inet_sport),
@@ -2619,11 +2619,10 @@ static void get_openreq4(const struct sock *sk, const struct request_sock *req,
 		0,  /* non standard timer */
 		0, /* open_requests have no inode */
 		atomic_read(&sk->sk_refcnt),
-		req,
-		len);
+		req);
 }
 
-static void get_tcp4_sock(struct sock *sk, struct seq_file *f, int i, int *len)
+static void get_tcp4_sock(struct sock *sk, struct seq_file *f, int i)
 {
 	int timer_active;
 	unsigned long timer_expires;
@@ -2662,7 +2661,7 @@ static void get_tcp4_sock(struct sock *sk, struct seq_file *f, int i, int *len)
 		rx_queue = max_t(int, tp->rcv_nxt - tp->copied_seq, 0);
 
 	seq_printf(f, "%4d: %08X:%04X %08X:%04X %02X %08X:%08X %02X:%08lX "
-			"%08X %5u %8d %lu %d %pK %lu %lu %u %u %d%n",
+			"%08X %5u %8d %lu %d %pK %lu %lu %u %u %d",
 		i, src, srcp, dest, destp, sk->sk_state,
 		tp->write_seq - tp->snd_una,
 		rx_queue,
@@ -2679,12 +2678,11 @@ static void get_tcp4_sock(struct sock *sk, struct seq_file *f, int i, int *len)
 		tp->snd_cwnd,
 		sk->sk_state == TCP_LISTEN ?
 		    (fastopenq ? fastopenq->max_qlen : 0) :
-		    (tcp_in_initial_slowstart(tp) ? -1 : tp->snd_ssthresh),
-		len);
+		    (tcp_in_initial_slowstart(tp) ? -1 : tp->snd_ssthresh));
 }
 
 static void get_timewait4_sock(const struct inet_timewait_sock *tw,
-			       struct seq_file *f, int i, int *len)
+			       struct seq_file *f, int i)
 {
 	__be32 dest, src;
 	__u16 destp, srcp;
@@ -2696,10 +2694,10 @@ static void get_timewait4_sock(const struct inet_timewait_sock *tw,
 	srcp  = ntohs(tw->tw_sport);
 
 	seq_printf(f, "%4d: %08X:%04X %08X:%04X"
-		" %02X %08X:%08X %02X:%08lX %08X %5d %8d %d %d %pK%n",
+		" %02X %08X:%08X %02X:%08lX %08X %5d %8d %d %d %pK",
 		i, src, srcp, dest, destp, tw->tw_substate, 0, 0,
 		3, jiffies_delta_to_clock_t(delta), 0, 0, 0, 0,
-		atomic_read(&tw->tw_refcnt), tw, len);
+		atomic_read(&tw->tw_refcnt), tw);
 }
 
 #define TMPSZ 150
@@ -2707,11 +2705,10 @@ static void get_timewait4_sock(const struct inet_timewait_sock *tw,
 static int tcp4_seq_show(struct seq_file *seq, void *v)
 {
 	struct tcp_iter_state *st;
-	int len;
 
+	seq_setwidth(seq, TMPSZ - 1);
 	if (v == SEQ_START_TOKEN) {
-		seq_printf(seq, "%-*s\n", TMPSZ - 1,
-			   "  sl  local_address rem_address   st tx_queue "
+		seq_puts(seq, "  sl  local_address rem_address   st tx_queue "
 			   "rx_queue tr tm->when retrnsmt   uid  timeout "
 			   "inode");
 		goto out;
@@ -2721,17 +2718,17 @@ static int tcp4_seq_show(struct seq_file *seq, void *v)
 	switch (st->state) {
 	case TCP_SEQ_STATE_LISTENING:
 	case TCP_SEQ_STATE_ESTABLISHED:
-		get_tcp4_sock(v, seq, st->num, &len);
+		get_tcp4_sock(v, seq, st->num);
 		break;
 	case TCP_SEQ_STATE_OPENREQ:
-		get_openreq4(st->syn_wait_sk, v, seq, st->num, st->uid, &len);
+		get_openreq4(st->syn_wait_sk, v, seq, st->num, st->uid);
 		break;
 	case TCP_SEQ_STATE_TIME_WAIT:
-		get_timewait4_sock(v, seq, st->num, &len);
+		get_timewait4_sock(v, seq, st->num);
 		break;
 	}
-	seq_printf(seq, "%*s\n", TMPSZ - 1 - len, "");
 out:
+	seq_pad(seq, '\n');
 	return 0;
 }
 
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 74d2c95..31c132c 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -2150,7 +2150,7 @@ EXPORT_SYMBOL(udp_proc_unregister);
 
 /* ------------------------------------------------------------------------ */
 static void udp4_format_sock(struct sock *sp, struct seq_file *f,
-		int bucket, int *len)
+		int bucket)
 {
 	struct inet_sock *inet = inet_sk(sp);
 	__be32 dest = inet->inet_daddr;
@@ -2159,7 +2159,7 @@ static void udp4_format_sock(struct sock *sp, struct seq_file *f,
 	__u16 srcp	  = ntohs(inet->inet_sport);
 
 	seq_printf(f, "%5d: %08X:%04X %08X:%04X"
-		" %02X %08X:%08X %02X:%08lX %08X %5u %8d %lu %d %pK %d%n",
+		" %02X %08X:%08X %02X:%08lX %08X %5u %8d %lu %d %pK %d",
 		bucket, src, srcp, dest, destp, sp->sk_state,
 		sk_wmem_alloc_get(sp),
 		sk_rmem_alloc_get(sp),
@@ -2167,23 +2167,22 @@ static void udp4_format_sock(struct sock *sp, struct seq_file *f,
 		from_kuid_munged(seq_user_ns(f), sock_i_uid(sp)),
 		0, sock_i_ino(sp),
 		atomic_read(&sp->sk_refcnt), sp,
-		atomic_read(&sp->sk_drops), len);
+		atomic_read(&sp->sk_drops));
 }
 
 int udp4_seq_show(struct seq_file *seq, void *v)
 {
+	seq_setwidth(seq, 127);
 	if (v == SEQ_START_TOKEN)
-		seq_printf(seq, "%-127s\n",
-			   "  sl  local_address rem_address   st tx_queue "
+		seq_puts(seq, "  sl  local_address rem_address   st tx_queue "
 			   "rx_queue tr tm->when retrnsmt   uid  timeout "
 			   "inode ref pointer drops");
 	else {
 		struct udp_iter_state *state = seq->private;
-		int len;
 
-		udp4_format_sock(v, seq, state->bucket, &len);
-		seq_printf(seq, "%*s\n", 127 - len, "");
+		udp4_format_sock(v, seq, state->bucket);
 	}
+	seq_pad(seq, '\n');
 	return 0;
 }
 
diff --git a/net/phonet/socket.c b/net/phonet/socket.c
index 77e38f7..008214a 100644
--- a/net/phonet/socket.c
+++ b/net/phonet/socket.c
@@ -595,26 +595,25 @@ static void pn_sock_seq_stop(struct seq_file *seq, void *v)
 
 static int pn_sock_seq_show(struct seq_file *seq, void *v)
 {
-	int len;
-
+	seq_setwidth(seq, 127);
 	if (v == SEQ_START_TOKEN)
-		seq_printf(seq, "%s%n", "pt  loc  rem rs st tx_queue rx_queue "
-			"  uid inode ref pointer drops", &len);
+		seq_puts(seq, "pt  loc  rem rs st tx_queue rx_queue "
+			"  uid inode ref pointer drops");
 	else {
 		struct sock *sk = v;
 		struct pn_sock *pn = pn_sk(sk);
 
 		seq_printf(seq, "%2d %04X:%04X:%02X %02X %08X:%08X %5d %lu "
-			"%d %pK %d%n",
+			"%d %pK %d",
 			sk->sk_protocol, pn->sobject, pn->dobject,
 			pn->resource, sk->sk_state,
 			sk_wmem_alloc_get(sk), sk_rmem_alloc_get(sk),
 			from_kuid_munged(seq_user_ns(seq), sock_i_uid(sk)),
 			sock_i_ino(sk),
 			atomic_read(&sk->sk_refcnt), sk,
-			atomic_read(&sk->sk_drops), &len);
+			atomic_read(&sk->sk_drops));
 	}
-	seq_printf(seq, "%*s\n", 127 - len, "");
+	seq_pad(seq, '\n');
 	return 0;
 }
 
@@ -785,20 +784,19 @@ static void pn_res_seq_stop(struct seq_file *seq, void *v)
 
 static int pn_res_seq_show(struct seq_file *seq, void *v)
 {
-	int len;
-
+	seq_setwidth(seq, 63);
 	if (v == SEQ_START_TOKEN)
-		seq_printf(seq, "%s%n", "rs   uid inode", &len);
+		seq_puts(seq, "rs   uid inode");
 	else {
 		struct sock **psk = v;
 		struct sock *sk = *psk;
 
-		seq_printf(seq, "%02X %5u %lu%n",
+		seq_printf(seq, "%02X %5u %lu",
 			   (int) (psk - pnres.sk),
 			   from_kuid_munged(seq_user_ns(seq), sock_i_uid(sk)),
-			   sock_i_ino(sk), &len);
+			   sock_i_ino(sk));
 	}
-	seq_printf(seq, "%*s\n", 63 - len, "");
+	seq_pad(seq, '\n');
 	return 0;
 }
 
diff --git a/net/sctp/objcnt.c b/net/sctp/objcnt.c
index 5ea573b..647396b 100644
--- a/net/sctp/objcnt.c
+++ b/net/sctp/objcnt.c
@@ -79,12 +79,13 @@ static sctp_dbg_objcnt_entry_t sctp_dbg_objcnt[] = {
  */
 static int sctp_objcnt_seq_show(struct seq_file *seq, void *v)
 {
-	int i, len;
+	int i;
 
 	i = (int)*(loff_t *)v;
-	seq_printf(seq, "%s: %d%n", sctp_dbg_objcnt[i].label,
-				atomic_read(sctp_dbg_objcnt[i].counter), &len);
-	seq_printf(seq, "%*s\n", 127 - len, "");
+	seq_setwidth(seq, 127);
+	seq_printf(seq, "%s: %d", sctp_dbg_objcnt[i].label,
+				atomic_read(sctp_dbg_objcnt[i].counter));
+	seq_pad(seq, '\n');
 	return 0;
 }
 
-- 
1.7.1

Re: [PATCH 1/2] remove all uses of printf's %n

[Tetsuo Handa]

Kees Cook wrote:
> -	seq_printf(m, "%s%d%n", con->name, con->index, &len);
> -	len = 21 - len;
> +	len = m->count;
> +	seq_printf(m, "%s%d", con->name, con->index);
> +	len = 21 - (m->count - len);

Why not to create a new function which returns bytes written?
The new function does not need to return negative value for indicating errors.
---------- patch start ----------
diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
index 4e32edc..c889cf1 100644
--- a/include/linux/seq_file.h
+++ b/include/linux/seq_file.h
@@ -91,6 +91,7 @@ int seq_write(struct seq_file *seq, const void *data, size_t len);
 
 __printf(2, 3) int seq_printf(struct seq_file *, const char *, ...);
 __printf(2, 0) int seq_vprintf(struct seq_file *, const char *, va_list args);
+__printf(2, 3) int seq_new_printf(struct seq_file *m, const char *f, ...);
 
 int seq_path(struct seq_file *, const struct path *, const char *);
 int seq_dentry(struct seq_file *, struct dentry *, const char *);
diff --git a/fs/seq_file.c b/fs/seq_file.c
index 3135c25..7af75ec 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -419,6 +419,27 @@ int seq_printf(struct seq_file *m, const char *f, ...)
 EXPORT_SYMBOL(seq_printf);
 
 /**
+ *	seq_new_printf - seq_printf() which returns bytes written.
+ *	@m:	target buffer
+ *	@f:	format
+ *
+ *      Returns bytes written to @m.
+ */
+int seq_new_printf(struct seq_file *m, const char *f, ...)
+{
+	const int count = m->count;
+	int ret;
+	va_list args;
+
+	va_start(args, f);
+	ret = seq_vprintf(m, f, args);
+	va_end(args);
+
+	return ret ? 0 : m->count - count;
+}
+EXPORT_SYMBOL(seq_new_printf);
+
+/**
  *	mangle_path -	mangle and copy path to buffer beginning
  *	@s: buffer start
  *	@p: beginning of path in above buffer
---------- patch end ----------
With new function, we can do:

-	len = m->count;
-	seq_printf(m, "%s%d", con->name, con->index);
-	len = 21 - (m->count - len);
+	len = 21 - seq_new_printf(m, "%s%d", con->name, con->index);

Re: [PATCH 1/2] remove all uses of printf's %n

[Kees Cook]

On Mon, Sep 16, 2013 at 4:41 AM, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
> Kees Cook wrote:
>> -     seq_printf(m, "%s%d%n", con->name, con->index, &len);
>> -     len = 21 - len;
>> +     len = m->count;
>> +     seq_printf(m, "%s%d", con->name, con->index);
>> +     len = 21 - (m->count - len);
>
> Why not to create a new function which returns bytes written?
> The new function does not need to return negative value for indicating errors.

I think it's not worth it for two reasons:

- there are very few callers that need this logic
- it would require a new function for each type of function used
(right now both seq_printf and seq_puts are used).

Perhaps instead of seq->count, there should be an access function?
seq_get_count(seq) or something?

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH 1/2] remove all uses of printf's %n

[Joe Perches]

On Mon, 2013-09-16 at 07:59 -0700, Kees Cook wrote:
> Perhaps instead of seq->count, there should be an access function?
> seq_get_count(seq) or something?

My thought was to add a seq_last_len()

Re: [PATCH 1/2] remove all uses of printf's %n

[Kees Cook]

On Mon, Sep 16, 2013 at 8:09 AM, Joe Perches <joe@perches.com> wrote:
> On Mon, 2013-09-16 at 07:59 -0700, Kees Cook wrote:
>> Perhaps instead of seq->count, there should be an access function?
>> seq_get_count(seq) or something?
>
> My thought was to add a seq_last_len()

That would mean growing the size of the seq_file structure and adding
instructions for all users. While I personally have no problem with
that, I worry others might. If we just use seq->count (or equivalent
function), then only those that want length will use it. I actually
think this uses fewer instructions than %n. Especially in the case
where seq_printf got replaced by seq_puts. :)

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH 1/2] remove all uses of printf's %n

[Joe Perches]

On Mon, 2013-09-16 at 08:25 -0700, Kees Cook wrote:
> On Mon, Sep 16, 2013 at 8:09 AM, Joe Perches <joe@perches.com> wrote:
> > On Mon, 2013-09-16 at 07:59 -0700, Kees Cook wrote:
> >> Perhaps instead of seq->count, there should be an access function?
> >> seq_get_count(seq) or something?
> >
> > My thought was to add a seq_last_len()
> 
> That would mean growing the size of the seq_file structure and adding
> instructions for all users. While I personally have no problem with
> that, I worry others might.

I don't think adding an int and a size_t is a big deal.

I'm still hoping to hear from Al if expanding the struct
is OK and race-free.

> If we just use seq->count (or equivalent
> function), then only those that want length will use it. I actually
> think this uses fewer instructions than %n. Especially in the case
> where seq_printf got replaced by seq_puts. :)

Shrug.  None of these are inline uses so the overall
code size doesn't change much.

I have patches that make seq_overflow public and replace
the current uses of the seq_printf/seq_puts/seq_putc
returns where appropriate.

Given that I was already touching a lot of the
seq_<foo> calls, I also have patches that convert all
the seq_printf(fmt) (no additional args) to seq_puts()
and all the seq_puts("[single char]") to seq_putc() as
separate patches.

Re: [PATCH 1/2] remove all uses of printf's %n

[George Spelvin]

> My thought was to add a seq_last_len()

In addition to adding per-call overhead to support a rarely-used feature
(while ->pos comes for free), this has the downside that it matters how
many separate calls are used to generate the string.

The advantage of the "read ->pos before and after" technique is that
you can have an arbitrary number of output calls in between.
Including things like seq_path(), seq_bitmap(), etc.

If you have the printing done in a subroutine (as in the net/ipv4
directory), it would be annoyingly subtle if seq_puts("foobar")
were not equivalent to seq_puts("foo"); seq_puts("bar").


I agree that exposing the internals via ->pos is a bit ugly, but too
many levels of abstraction makes code hard to read, too, and it's very
straightforward to find and fix the dozen or so places where it's accessed
in the unlikely case that the internals of seq_file change significantly.

My take on it is "not worth fixing".

Re: [PATCH 1/2] remove all uses of printf's %n

[Joe Perches]

On Mon, 2013-09-16 at 13:21 -0400, George Spelvin wrote:
> > My thought was to add a seq_last_len()
> 
> In addition to adding per-call overhead to support a rarely-used feature
> (while ->pos comes for free), this has the downside that it matters how
> many separate calls are used to generate the string.
> 
> The advantage of the "read ->pos before and after" technique is that
> you can have an arbitrary number of output calls in between.
> Including things like seq_path(), seq_bitmap(), etc.
> 
> If you have the printing done in a subroutine (as in the net/ipv4
> directory), it would be annoyingly subtle if seq_puts("foobar")
> were not equivalent to seq_puts("foo"); seq_puts("bar").

It's already true that those aren't equivalent.
	seq_puts(seq, "short ");
	set_puts(seq, "string");
may fit the first bit into the output buffer but not the last
where
	setq_puts(seq, "short string");
would not fit at all.

btw: who are you?  Is your name really George Spelvin?

Re: [PATCH 1/2] remove all uses of printf's %n

[George Spelvin]

> All users of %n are calculating padding size when using seq_file, so
> instead use the new last_len member for discovering the length of the
> written strings.

Obviously, this comment needs to be updated, but once that is done,
Acked-by: George Spelvin <linux@horizon.com>.

I actually reviewed all the users and checked that things work
properly.  In the various VMA map and tcp state lists, the line
width (128 or the now-misnamed TMPSZ=150) is greatly in excess of
what's required, presumably to facilitate computing the iteration
index from the file position in pre-seqfile days.

If the line were to ever get longer than that, the semantics of
printf("%*s", -5, "") would give strange results, but that's
a pre-existing problem that is unlikely to occur, would probably
be harmless (as would deleting the trailing whitespace!), and
isn't made any worse by this.


HOWEVER, it gave me an idea.  We could put this padding logic straight
into vsprintf and clean up all the callers.

I'm currently thinking that a precision specifier in %c (%.*c) is
currently meaningless and ignored.  What if we usurped it and defined
it to mean "pad with this character until the total width so far this
rint reaches the specified precision"?

That would DTRT for all the callers.

Oh, bugger, gcc -Wformat warns about doing that.  And %#127c.  And
everything else I can think of.  The only way to sneak it past gcc
would be to usurp the rarely-used %-127c format.

Since the only pad character currently used is space, we could omit the
argument and use something like %127%, but gcc gets even more confused
by that.

The code is easy enough.  But any suggestions for ways to represent it
in the format string would be appreciated.

Re: [PATCH 1/2] remove all uses of printf's %n

[Joe Perches]

On Mon, 2013-09-16 at 12:07 -0400, George Spelvin wrote:
> it gave me an idea.  We could put this padding logic straight
> into vsprintf and clean up all the callers.
[]
> Since the only pad character currently used is space, we could omit the
> argument and use something like %127%, but gcc gets even more confused
> by that.
> 
> The code is easy enough.  But any suggestions for ways to represent it
> in the format string would be appreciated.

%*p<new_type><pad_char> (space assumed if not existing)

Re: [PATCH 1/2] remove all uses of printf's %n

[George Spelvin]

> %*p<new_type><pad_char> (space assumed if not existing)

Yes, that's the fallback, but it requires an ugly dummy pointer argument.
And some extra kludgery in the code because pointer() doesn't have access
to the start-of-buffer address.

I'd prefer something that could be detected with the information available
in the switch(spec.type) in vsnprintf() itself.

It is, however, available as a last resort.

Re: [PATCH 1/2] remove all uses of printf's %n

[Joe Perches]

On Mon, 2013-09-16 at 12:39 -0400, George Spelvin wrote:
> > %*p<new_type><pad_char> (space assumed if not existing)
> 
> Yes, that's the fallback, but it requires an ugly dummy pointer argument.
> And some extra kludgery in the code because pointer() doesn't have access
> to the start-of-buffer address.
>
> I'd prefer something that could be detected with the information available
> in the switch(spec.type) in vsnprintf() itself.

Bad argument I think.

It'd be consistent with all the other %p<foo> types.

vsnprintf is already weird enough with %p uses,
there's absolutely no reason to stretch it further
with yet another odd access/format style.

Re: [PATCH 1/2] remove all uses of printf's %n

[George Spelvin]

> It'd be consistent with all the other %p<foo> types.
>
> vsnprintf is already weird enough with %p uses,
> there's absolutely no reason to stretch it further
> with yet another odd access/format style.

Well, all the other %p<foo> types actually *use* the void * argument.
They print the thing pointed to, just in different ways.

What I'm proposing is fundamentally different, and much more
"printf internals" specific.

I hate creating an interface that requires a dummy pointer argument.

This would already be quite different in , and I hate creating a
new interface that requires a dummy pointer argument.


One nonsensical combination that gcc does *not* complain about
is "%0-c".  (It does bitch about "%-0c", however.)

Is "%0-127c" too ugly to live?

Note that I could generalize it, and allow "%0-<width>" to mean
"left-align, with trailing padding to column <width>" for ANY format
specifier if that would help.  It's easy in the printf code to compute
the field width necessary to make that work.
ANSI C says that %0- is permitted and means the same thing as %-
(- overrides 0), but that could be bent.

Re: [PATCH 1/2] remove all uses of printf's %n

[Joe Perches]

On Mon, 2013-09-16 at 15:15 -0400, George Spelvin wrote:
> > It'd be consistent with all the other %p<foo> types.
> >
> > vsnprintf is already weird enough with %p uses,
> > there's absolutely no reason to stretch it further
> > with yet another odd access/format style.
> 
> Well, all the other %p<foo> types actually *use* the void * argument.
> They print the thing pointed to, just in different ways.
> 
> What I'm proposing is fundamentally different, and much more
> "printf internals" specific.
> 
> I hate creating an interface that requires a dummy pointer argument.
[]
> Is "%0-127c" too ugly to live?

Yes.

I suppose you use "%[-]*pV" if you really want funky/fugly.

[PATCH 2/2] vsprintf: ignore %n again

[Kees Cook]

This ignores %n in printf again, as was originally documented. Implementing
%n poses a greater security risk than utility, so it should stay ignored.
To help anyone attempting to use %n, a warning will be emitted if it is
encountered.

Based on earlier patch by Joe Perches.

Signed-off-by: Kees Cook <keescook@chromium.org>
Cc: Joe Perches <joe@perches.com>
---
 lib/vsprintf.c |   20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 26559bd..478c30e 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1683,18 +1683,16 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 			break;
 
 		case FORMAT_TYPE_NRCHARS: {
-			u8 qualifier = spec.qualifier;
+			/*
+			 * Since %n poses a greater security risk than
+			 * utility, ignore %n and skip its argument.
+			 */
+			void *skip_arg;
 
-			if (qualifier == 'l') {
-				long *ip = va_arg(args, long *);
-				*ip = (str - buf);
-			} else if (_tolower(qualifier) == 'z') {
-				size_t *ip = va_arg(args, size_t *);
-				*ip = (str - buf);
-			} else {
-				int *ip = va_arg(args, int *);
-				*ip = (str - buf);
-			}
+			WARN_ONCE(1, "Please remove ignored %%n in '%s'\n",
+					old_fmt);
+
+			skip_arg = va_arg(args, void *);
 			break;
 		}
 
-- 
1.7.9.5

Re: [PATCH 0/2] vsprintf: ignore %n again

[Al Viro]

On Mon, Sep 16, 2013 at 12:43:55AM -0700, Kees Cook wrote:
> Whether seq_printf should return void or error, %n still needs to be removed.
> As such, instead of changing the seq_file structure and adding instructions
> to all callers of seq_printf, just examine seq->count for the callers that
> care about how many characters were put into the buffer, as suggested by
> George Spelvin. First patch removes all %n usage in favor of checking
> seq->count before/after. Second patch makes %n ignore its argument.

This is completely pointless.  *ANY* untrusted format string kernel-side
is pretty much it.  Blocking %n is not "defense in depth", it's security
theater.  Again, if attacker can feed an arbitrary format string to
vsnprintf(), it's over - you've lost.  It's not just about information
leaks vs. ability to store a value of attacker's choosing at the address
of attacker's choosing as it was in userland.  Kernel-side an ability to
trigger read from an arbitrary address is much nastier than information
leak risk; consider iomem, for starters.

What we ought to do is prevention of _that_.  AFAICS, we have reasonably
few call chains that might transmit format string; most of the calls
are with plain and simple string literal.  I wonder if could get away
with reasonable amount of annotations to catch such crap...

Consider, e.g. introducing __vsnprint(), with vsnprintf(s, n, fmt, ...)
expanding to __vsnprintf(1, s, n, fmt, ...) if fmt is a string literal
and __vsnprintf(0, s, n, fmt, ...) otherwise.  Now,
int __sprintf(int safe, char *buf, const char *fmt, ...)
{
        va_list args;
        int i;

        va_start(args, fmt);
        i = __vsnprintf(safe, buf, INT_MAX, fmt, args);
        va_end(args);

        return i;
}
and #define for sprintf (expanding it to either __sprintf(1, ...)
or __sprintf(0, ...)).  That plus similar for snprintf and seq_printf
will already take care of most of the call chains leading to __vsnprintf() -
relatively few calls with have 0 passed to it.  Add WARN_ON(!safe) to
__vsnprintf and we probably won't drown in warnings.  Now, we can start
adding things like that to remaining call chains *and* do things like
replacing
                        snd_iprintf(buffer, fields[i].format,
                                *get_dummy_ll_ptr(dummy, fields[i].offset));
with
			/* fields[i].format is known to be a valid format */
                        __snd_iprintf(1, buffer, fields[i].format,
                                *get_dummy_ll_ptr(dummy, fields[i].offset));
to deal with the places where the origin of format string is provably safe,
but not a string literal (actually, s/1/__FORMAT_IS_SAFE/, to make it
greppable).

Comments?

Re: [PATCH 0/2] vsprintf: ignore %n again

[Lars-Peter Clausen]

On 09/16/2013 05:55 PM, Al Viro wrote:
> On Mon, Sep 16, 2013 at 12:43:55AM -0700, Kees Cook wrote:
>> Whether seq_printf should return void or error, %n still needs to be removed.
>> As such, instead of changing the seq_file structure and adding instructions
>> to all callers of seq_printf, just examine seq->count for the callers that
>> care about how many characters were put into the buffer, as suggested by
>> George Spelvin. First patch removes all %n usage in favor of checking
>> seq->count before/after. Second patch makes %n ignore its argument.
> 
> This is completely pointless.  *ANY* untrusted format string kernel-side
> is pretty much it.  Blocking %n is not "defense in depth", it's security
> theater.  Again, if attacker can feed an arbitrary format string to
> vsnprintf(), it's over - you've lost.  It's not just about information
> leaks vs. ability to store a value of attacker's choosing at the address
> of attacker's choosing as it was in userland.  Kernel-side an ability to
> trigger read from an arbitrary address is much nastier than information
> leak risk; consider iomem, for starters.
> 
> What we ought to do is prevention of _that_.  AFAICS, we have reasonably
> few call chains that might transmit format string; most of the calls
> are with plain and simple string literal.  I wonder if could get away
> with reasonable amount of annotations to catch such crap...
> 
> Consider, e.g. introducing __vsnprint(), with vsnprintf(s, n, fmt, ...)
> expanding to __vsnprintf(1, s, n, fmt, ...) if fmt is a string literal
> and __vsnprintf(0, s, n, fmt, ...) otherwise.  Now,
> int __sprintf(int safe, char *buf, const char *fmt, ...)
> {
>         va_list args;
>         int i;
> 
>         va_start(args, fmt);
>         i = __vsnprintf(safe, buf, INT_MAX, fmt, args);
>         va_end(args);
> 
>         return i;
> }
> and #define for sprintf (expanding it to either __sprintf(1, ...)
> or __sprintf(0, ...)).  That plus similar for snprintf and seq_printf
> will already take care of most of the call chains leading to __vsnprintf() -
> relatively few calls with have 0 passed to it.  Add WARN_ON(!safe) to
> __vsnprintf and we probably won't drown in warnings.  Now, we can start
> adding things like that to remaining call chains *and* do things like
> replacing
>                         snd_iprintf(buffer, fields[i].format,
>                                 *get_dummy_ll_ptr(dummy, fields[i].offset));
> with
> 			/* fields[i].format is known to be a valid format */
>                         __snd_iprintf(1, buffer, fields[i].format,
>                                 *get_dummy_ll_ptr(dummy, fields[i].offset));
> to deal with the places where the origin of format string is provably safe,
> but not a string literal (actually, s/1/__FORMAT_IS_SAFE/, to make it
> greppable).
> 
> Comments?

I wrote a script the other day, which first recursively collects functions
that somehow end up passing a format string to vsnprintf. And then as a
second step finds all invocations of these functions with a non-const
string. As far as I can tell callers of vsnprintf and friends usually get it
right, it's rather functions that call a function that calls a function that
calls vsnprintf that get misused (There is one subsystem which seems to be
Swiss cheese in regard to this). So doing this just for a few functions
won't help you'd have to do this for all functions that take format strings.

- Lars

Re: [PATCH 0/2] vsprintf: ignore %n again

[George Spelvin]

> This is completely pointless.  *ANY* untrusted format string kernel-side
> is pretty much it.  Blocking %n is not "defense in depth", it's security
> theater.  Again, if attacker can feed an arbitrary format string to
> vsnprintf(), it's over - you've lost.  It's not just about information
> leaks vs. ability to store a value of attacker's choosing at the address
> of attacker's choosing as it was in userland.  Kernel-side an ability to
> trigger read from an arbitrary address is much nastier than information
> leak risk; consider iomem, for starters.

You've got to be kidding.  Yes, sometimes a read can have effects, but
such addresses are rare, not present in the main kernel memory mapping,
and you'd have to find a pointer to such an address (or a preceding
address with no nul bytes in between) on the stack at a known offset when
designing the printf string.  That's tricky, and not always possible.
Even for hardware devices, read side effects have gone out of style,
other than forcing PCI posted writes through.

And just because a hardware read has side effects doesn't mean it's
exploitable.  Being able to drop characters from a serial port is only
a DoS.

On the other hand, the ability to write an arbitrary, attacker-controlled
small integer to any address findable on the stack is very powerful and
easy to exploit.

Those two risks aren't remotely equivalent.

Re: [PATCH 0/2] vsprintf: ignore %n again

[Kees Cook]

On Mon, Sep 16, 2013 at 8:55 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Mon, Sep 16, 2013 at 12:43:55AM -0700, Kees Cook wrote:
>> Whether seq_printf should return void or error, %n still needs to be removed.
>> As such, instead of changing the seq_file structure and adding instructions
>> to all callers of seq_printf, just examine seq->count for the callers that
>> care about how many characters were put into the buffer, as suggested by
>> George Spelvin. First patch removes all %n usage in favor of checking
>> seq->count before/after. Second patch makes %n ignore its argument.
>
> This is completely pointless.  *ANY* untrusted format string kernel-side
> is pretty much it.  Blocking %n is not "defense in depth", it's security
> theater.  Again, if attacker can feed an arbitrary format string to
> vsnprintf(), it's over - you've lost.  It's not just about information
> leaks vs. ability to store a value of attacker's choosing at the address
> of attacker's choosing as it was in userland.  Kernel-side an ability to
> trigger read from an arbitrary address is much nastier than information
> leak risk; consider iomem, for starters.

I completely disagree. (Surprise!) Eliminating %n _does_ change
things. Why have the risk when solutions for getting the same behavior
take fewer instructions and are safer? Yes, I agree that arbitrary
format strings are still a very very bad thing, but %n makes it an
order of magnitude worse. Attacking format strings with %n is very
well understood by exploiters, so why give them this primitive for no
sensible reason?

> What we ought to do is prevention of _that_.  AFAICS, we have reasonably
> few call chains that might transmit format string; most of the calls
> are with plain and simple string literal.  I wonder if could get away
> with reasonable amount of annotations to catch such crap...

Yes. And if gcc would ignore const char arguments, we could do this:

diff --git a/Makefile b/Makefile
index e73f758..2bc680b 100644
--- a/Makefile
+++ b/Makefile
@@ -372,7 +372,6 @@ KBUILD_CPPFLAGS := -D__KERNEL__
 KBUILD_CFLAGS   := -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs \
                   -fno-strict-aliasing -fno-common \
                   -Werror-implicit-function-declaration \
-                  -Wno-format-security \
                   -fno-delete-null-pointer-checks
 KBUILD_AFLAGS_KERNEL :=
 KBUILD_CFLAGS_KERNEL :=
@@ -659,6 +658,11 @@ KBUILD_CFLAGS      += $(call cc-option,-fno-strict-overflow
 # conserve stack if available
 KBUILD_CFLAGS   += $(call cc-option,-fconserve-stack)

+# Enable format-security when it can stop the build, otherwise disable.
+KBUILD_CFLAGS  += $(call cc-option,\
+                       -Wformat -Wformat-security -Werror=format-security,\
+                       -Wno-format-security)
+
 # use the deterministic mode of AR if available
 KBUILD_ARFLAGS := $(call ar-option,D)

I have a patch series that fixes all the warnings produced by gcc in
this state. All of our printf uses are already marked up in the
kernel, so this checks them all.

> Consider, e.g. introducing __vsnprint(), with vsnprintf(s, n, fmt, ...)
> expanding to __vsnprintf(1, s, n, fmt, ...) if fmt is a string literal
> and __vsnprintf(0, s, n, fmt, ...) otherwise.  Now,
> int __sprintf(int safe, char *buf, const char *fmt, ...)
> {
>         va_list args;
>         int i;
>
>         va_start(args, fmt);
>         i = __vsnprintf(safe, buf, INT_MAX, fmt, args);
>         va_end(args);
>
>         return i;
> }
> and #define for sprintf (expanding it to either __sprintf(1, ...)
> or __sprintf(0, ...)).  That plus similar for snprintf and seq_printf
> will already take care of most of the call chains leading to __vsnprintf() -
> relatively few calls with have 0 passed to it.  Add WARN_ON(!safe) to
> __vsnprintf and we probably won't drown in warnings.  Now, we can start
> adding things like that to remaining call chains *and* do things like
> replacing
>                         snd_iprintf(buffer, fields[i].format,
>                                 *get_dummy_ll_ptr(dummy, fields[i].offset));
> with
>                         /* fields[i].format is known to be a valid format */
>                         __snd_iprintf(1, buffer, fields[i].format,
>                                 *get_dummy_ll_ptr(dummy, fields[i].offset));
> to deal with the places where the origin of format string is provably safe,
> but not a string literal (actually, s/1/__FORMAT_IS_SAFE/, to make it
> greppable).

Unless I've misunderstood, I think we'd already get very close to this
with the gcc options instead. This patch set is what I've been using
to generate the format string fixes over the last few months, with 7
sent this last round:

http://git.kernel.org/cgit/linux/kernel/git/kees/linux.git/log/?h=format-security

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH 0/2] vsprintf: ignore %n again

[Tetsuo Handa]

Kees Cook wrote:
> > Consider, e.g. introducing __vsnprint(), with vsnprintf(s, n, fmt, ...)
> > expanding to __vsnprintf(1, s, n, fmt, ...) if fmt is a string literal
> > and __vsnprintf(0, s, n, fmt, ...) otherwise.  Now,
> > int __sprintf(int safe, char *buf, const char *fmt, ...)
> > {
> >         va_list args;
> >         int i;
> >
> >         va_start(args, fmt);
> >         i = __vsnprintf(safe, buf, INT_MAX, fmt, args);
> >         va_end(args);
> >
> >         return i;
> > }
> 
> Unless I've misunderstood, I think we'd already get very close to this
> with the gcc options instead. This patch set is what I've been using
> to generate the format string fixes over the last few months, with 7
> sent this last round:

Can we utilize __builtin_constant_p() ?

---------- source start ----------
#include <stdio.h>

#define func(fmt)                                       \
        if (__builtin_constant_p(fmt))                  \
                printf("const : %s\n", fmt);            \
        else                                            \
                printf("not const : %s\n", fmt);        \


int main(int argc, char *argv[])
{
        const char *fmt1 = "const char *";
        const char fmt2[] = "const char []";
        const char * const fmt3 = "const char * const";
        func("literal");
        func(fmt1);
        func(fmt2);
        func(fmt3);
        return 0;
}
---------- source end ----------

---------- output start ----------
const : literal
not const : const char *
not const : const char []
const : const char * const
---------- output end ----------

__builtin_constant_p() seems to enforce use of either "literal" or "* const".

An example change

---------- patch start ----------
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -120,8 +120,9 @@ asmlinkage int printk_emit(int facility, int level,
                           const char *dict, size_t dictlen,
                           const char *fmt, ...);
 
-asmlinkage __printf(1, 2) __cold
-int printk(const char *fmt, ...);
+//asmlinkage __printf(1, 2) __cold
+//int printk(const char *fmt, ...);
+#define printk(fmt, ...) compiletime_assert(__builtin_constant_p(fmt), "Non-c  onstant format string")
 
 /*
  * Special printk facility for scheduler use only, _DO_NOT_USE_ !
---------- patch end ----------

caught errors like below.

  CC [M]  drivers/scsi/esas2r/esas2r_log.o
drivers/scsi/esas2r/esas2r_log.c: In function 'esas2r_log_master':
drivers/scsi/esas2r/esas2r_log.c:174: error: call to '__compiletime_assert_174' declared with attribute error: Non-constant format string

Re: [PATCH 0/2] vsprintf: ignore %n again

[Dan Carpenter]

Sure.  There are a lot of non-const strings though.

diff --git a/include/linux/printk.h b/include/linux/printk.h
index e6131a78..317587b 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -122,6 +122,11 @@ asmlinkage int printk_emit(int facility, int level,
 
 asmlinkage __printf(1, 2) __cold
 int printk(const char *fmt, ...);
+#define printk(fmt, ...) do {				  \
+	compiletime_assert(__builtin_constant_p(fmt),     \
+			   "Non-constant format string"); \
+	printk(fmt, ##__VA_ARGS__);			  \
+} while (0)
 
 /*
  * Special printk facility for scheduler use only, _DO_NOT_USE_ !

Re: [PATCH 0/2] vsprintf: ignore %n again

[Dan Carpenter]

On Wed, Sep 18, 2013 at 05:11:04PM +0300, Dan Carpenter wrote:
>  asmlinkage __printf(1, 2) __cold
>  int printk(const char *fmt, ...);
> +#define printk(fmt, ...) do {				  \
> +	compiletime_assert(__builtin_constant_p(fmt),     \
> +			   "Non-constant format string"); \
> +	printk(fmt, ##__VA_ARGS__);			  \
> +} while (0)

Oops...  That breaks the compile for printk.c.

Also don't we want 'const char fmt[] = "foo %s";' to be a valid format
string?

regards,
dan carpenter

Re: [PATCH 0/2] vsprintf: ignore %n again

[George Spelvin]

> +#define printk(fmt, ...) do {				  \
> +	compiletime_assert(__builtin_constant_p(fmt),     \
> +			   "Non-constant format string"); \
> +	printk(fmt, ##__VA_ARGS__);			  \
> +} while (0)

May I recommend __builtin_constant_p(*fmt).  Since:

char buf[OVERFLOW_VULNERABILITY];
strcpy(buf, malicious_format);
printk(buf, args)

has __builtin_constant_p(buf), but it's not a constant format string.

Re: [PATCH 0/2] vsprintf: ignore %n again

[Kees Cook]

On Wed, Sep 18, 2013 at 6:14 AM, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
> Kees Cook wrote:
>> > Consider, e.g. introducing __vsnprint(), with vsnprintf(s, n, fmt, ...)
>> > expanding to __vsnprintf(1, s, n, fmt, ...) if fmt is a string literal
>> > and __vsnprintf(0, s, n, fmt, ...) otherwise.  Now,
>> > int __sprintf(int safe, char *buf, const char *fmt, ...)
>> > {
>> >         va_list args;
>> >         int i;
>> >
>> >         va_start(args, fmt);
>> >         i = __vsnprintf(safe, buf, INT_MAX, fmt, args);
>> >         va_end(args);
>> >
>> >         return i;
>> > }
>>
>> Unless I've misunderstood, I think we'd already get very close to this
>> with the gcc options instead. This patch set is what I've been using
>> to generate the format string fixes over the last few months, with 7
>> sent this last round:
>
> Can we utilize __builtin_constant_p() ?

This seems promising.

>
> ---------- source start ----------
> #include <stdio.h>
>
> #define func(fmt)                                       \
>         if (__builtin_constant_p(fmt))                  \
>                 printf("const : %s\n", fmt);            \
>         else                                            \
>                 printf("not const : %s\n", fmt);        \
>
>
> int main(int argc, char *argv[])
> {
>         const char *fmt1 = "const char *";
>         const char fmt2[] = "const char []";
>         const char * const fmt3 = "const char * const";
>         func("literal");
>         func(fmt1);
>         func(fmt2);
>         func(fmt3);
>         return 0;
> }
> ---------- source end ----------
>
> ---------- output start ----------
> const : literal
> not const : const char *
> not const : const char []
> const : const char * const

What version of gcc did you use? I don't get the last as const, for
some reason. And as Dan mentions, shouldn't const char[] be detected
as const too?

-Kees

> ---------- output end ----------
>
> __builtin_constant_p() seems to enforce use of either "literal" or "* const".
>
> An example change
>
> ---------- patch start ----------
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -120,8 +120,9 @@ asmlinkage int printk_emit(int facility, int level,
>                            const char *dict, size_t dictlen,
>                            const char *fmt, ...);
>
> -asmlinkage __printf(1, 2) __cold
> -int printk(const char *fmt, ...);
> +//asmlinkage __printf(1, 2) __cold
> +//int printk(const char *fmt, ...);
> +#define printk(fmt, ...) compiletime_assert(__builtin_constant_p(fmt), "Non-c  onstant format string")
>
>  /*
>   * Special printk facility for scheduler use only, _DO_NOT_USE_ !
> ---------- patch end ----------
>
> caught errors like below.
>
>   CC [M]  drivers/scsi/esas2r/esas2r_log.o
> drivers/scsi/esas2r/esas2r_log.c: In function 'esas2r_log_master':
> drivers/scsi/esas2r/esas2r_log.c:174: error: call to '__compiletime_assert_174' declared with attribute error: Non-constant format string



-- 
Kees Cook
Chrome OS Security

Re: [PATCH 0/2] vsprintf: ignore %n again

[Tetsuo Handa]

Kees Cook wrote:
> > ---------- output start ----------
> > const : literal
> > not const : const char *
> > not const : const char []
> > const : const char * const
> 
> What version of gcc did you use? I don't get the last as const, for
> some reason. And as Dan mentions, shouldn't const char[] be detected
> as const too?

This worked on

  gcc (GCC) 4.4.7 20120313 (Red Hat 4.4.7-3)
  gcc (GCC) 3.3.5 (Debian 1:3.3.5-13)

with -On (where n != 0), but didn't work on

  gcc (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3

. Oops...

Re: [PATCH 0/2] vsprintf: ignore %n again

[Tetsuo Handa]

If the code to test is built into vmlinux, we could use run-time checking like

----------
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1601,6 +1601,11 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 	if (WARN_ON_ONCE((int) size < 0))
 		return 0;
 
+	if (!(__start_rodata <= fmt && fmt < __end_rodata)) {
+		static unsigned char warn = 100;
+		WARN(warn && warn--, "Format string is not in RODATA section.");
+	}
+
 	str = buf;
 	end = buf + size;
 
----------

which reports errors like below.

[    0.814121] ------------[ cut here ]------------
[    0.814985] WARNING: CPU: 0 PID: 1 at lib/vsprintf.c:1606 vsnprintf+0xb4/0x3f0()
[    0.816036] Format string is not in RODATA section.
[    0.816883] Modules linked in:
[    0.817490] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.12.0-rc1-00046-g9baa505-dirty #180
[    0.818974] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007
[    0.820040]  00000646 00000000 df079dc0 c02eae66 df079dcc c02eaead c02f3684 df079df8
[    0.821538]  c01422a9 c061bc40 df079e28 00000001 c061bc31 00000646 c02f3684 df079e08
[    0.822995]  00000646 c061bc40 df079e14 c0142301 00000009 df079e08 c061bc40 df079e28
[    0.824676] Call Trace:
[    0.825124]  [<c02eae66>] __dump_stack+0x16/0x20
[    0.825922]  [<c02eaead>] dump_stack+0x3d/0x60
[    0.826691]  [<c02f3684>] ? vsnprintf+0xb4/0x3f0
[    0.827478]  [<c01422a9>] warn_slowpath_common+0x79/0xa0
[    0.828040]  [<c02f3684>] ? vsnprintf+0xb4/0x3f0
[    0.828917]  [<c0142301>] warn_slowpath_fmt+0x31/0x40
[    0.829798]  [<c02f3684>] vsnprintf+0xb4/0x3f0
[    0.830584]  [<c0199d2b>] ? trace_hardirqs_on+0xb/0x10
[    0.832050]  [<c02f70f4>] kvasprintf+0x24/0x60
[    0.832831]  [<c02ed1c1>] kobject_set_name_vargs+0x21/0x60
[    0.833850]  [<c02ed2c1>] kobject_add_varg+0x21/0x50
[    0.834750]  [<c02ed379>] kobject_init_and_add+0x29/0x30
[    0.835699]  [<c01f9fd3>] sysfs_slab_add+0x63/0xe0
[    0.836055]  [<c06ec630>] ? kmem_cache_init_late+0x10/0x10
[    0.837054]  [<c06ec6a7>] slab_sysfs_init+0x77/0x110
[    0.838025]  [<c06ec251>] ? procswaps_init+0x21/0x30
[    0.838958]  [<c0100472>] do_one_initcall+0x32/0xd0
[    0.840046]  [<c015fb60>] ? parse_one+0xc0/0xe0
[    0.840852]  [<c015fd0a>] ? parse_args+0x7a/0x170
[    0.841676]  [<c06cf410>] ? loglevel+0x30/0x30
[    0.842443]  [<c06cfb6a>] do_initcall_level+0x7a/0x90
[    0.843338]  [<c06cf410>] ? loglevel+0x30/0x30
[    0.844038]  [<c06cfb98>] do_initcalls+0x18/0x20
[    0.844960]  [<c06cfbc8>] do_basic_setup+0x28/0x30
[    0.845894]  [<c06cfc7f>] kernel_init_freeable+0x5f/0xf0
[    0.846907]  [<c04c523b>] kernel_init+0xb/0xe0
[    0.848045]  [<c04cc718>] ret_from_kernel_thread+0x1c/0x2c
[    0.849070]  [<c04c5230>] ? rest_init+0x140/0x140
[    0.849987] ---[ end trace c57fc7b42d34a992 ]---

----------
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -5136,7 +5136,7 @@ static int sysfs_slab_add(struct kmem_cache *s)
 	}
 
 	s->kobj.kset = slab_kset;
-	err = kobject_init_and_add(&s->kobj, &slab_ktype, NULL, name);
+	err = kobject_init_and_add(&s->kobj, &slab_ktype, NULL, "%s", name);
 	if (err) {
 		kobject_put(&s->kobj);
 		return err;
----------

But tools like sparse might find such bugs better?

Re: [PATCH 0/2] vsprintf: ignore %n again

[Kees Cook]

On Wed, Sep 18, 2013 at 6:14 AM, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
> Kees Cook wrote:
>> > Consider, e.g. introducing __vsnprint(), with vsnprintf(s, n, fmt, ...)
>> > expanding to __vsnprintf(1, s, n, fmt, ...) if fmt is a string literal
>> > and __vsnprintf(0, s, n, fmt, ...) otherwise.  Now,
>> > int __sprintf(int safe, char *buf, const char *fmt, ...)
>> > {
>> >         va_list args;
>> >         int i;
>> >
>> >         va_start(args, fmt);
>> >         i = __vsnprintf(safe, buf, INT_MAX, fmt, args);
>> >         va_end(args);
>> >
>> >         return i;
>> > }
>>
>> Unless I've misunderstood, I think we'd already get very close to this
>> with the gcc options instead. This patch set is what I've been using
>> to generate the format string fixes over the last few months, with 7
>> sent this last round:
>
> Can we utilize __builtin_constant_p() ?
>
> ---------- source start ----------
> #include <stdio.h>
>
> #define func(fmt)                                       \
>         if (__builtin_constant_p(fmt))                  \
>                 printf("const : %s\n", fmt);            \
>         else                                            \
>                 printf("not const : %s\n", fmt);        \
>
>
> int main(int argc, char *argv[])
> {
>         const char *fmt1 = "const char *";
>         const char fmt2[] = "const char []";
>         const char * const fmt3 = "const char * const";
>         func("literal");
>         func(fmt1);
>         func(fmt2);
>         func(fmt3);

I wonder if we can get at the read-only knowledge. Doing this shows
that gcc knows they're read-only:

fmt1[2] = 'A';
fmt2[2] = 'A';
fmt3[2] = 'A';

/tmp/const.c:27:5: error: assignment of read-only location ‘*(fmt1 + 2u)’
/tmp/const.c:29:5: error: assignment of read-only location ‘fmt2[2]’
/tmp/const.c:31:5: error: assignment of read-only location ‘*(fmt3 + 2u)’

I didn't see a builtin for this in the gcc documentation I've been reading.

-Kees

>         return 0;
> }
> ---------- source end ----------
>
> ---------- output start ----------
> const : literal
> not const : const char *
> not const : const char []
> const : const char * const
> ---------- output end ----------
>
> __builtin_constant_p() seems to enforce use of either "literal" or "* const".
>
> An example change
>
> ---------- patch start ----------
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -120,8 +120,9 @@ asmlinkage int printk_emit(int facility, int level,
>                            const char *dict, size_t dictlen,
>                            const char *fmt, ...);
>
> -asmlinkage __printf(1, 2) __cold
> -int printk(const char *fmt, ...);
> +//asmlinkage __printf(1, 2) __cold
> +//int printk(const char *fmt, ...);
> +#define printk(fmt, ...) compiletime_assert(__builtin_constant_p(fmt), "Non-c  onstant format string")
>
>  /*
>   * Special printk facility for scheduler use only, _DO_NOT_USE_ !
> ---------- patch end ----------
>
> caught errors like below.
>
>   CC [M]  drivers/scsi/esas2r/esas2r_log.o
> drivers/scsi/esas2r/esas2r_log.c: In function 'esas2r_log_master':
> drivers/scsi/esas2r/esas2r_log.c:174: error: call to '__compiletime_assert_174' declared with attribute error: Non-constant format string



-- 
Kees Cook
Chrome OS Security