[RFC PATCH] vsnprintf: Remove use of %n and convert existing uses

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Using vsnprintf or its derivatives with %n can have security
vulnerability implications.

Prior to commit fef20d9c1380
("vsprintf: unify the format decoding layer for its 3 users"),
any use of %n was ignored.

Reintroduce this feature and convert the existing uses of %n
to use the return length from vsnprintf or its derivatives.

Signed-off-by: Joe Perches <joe@xxxxxxxxxxx>
Acked-by: KOSAKI Motohiro <kosaki.motohiro@xxxxxxxxxxxxxx> (proc bits)
cc: Kees Cook <keescook@xxxxxxxxxxxx>
cc: Frederic Weisbecker <fweisbec@xxxxxxxxx>

---

Not particularly well tested...

 fs/proc/consoles.c   |  2 +-
 fs/proc/nommu.c      | 20 ++++++-------
 fs/proc/task_mmu.c   | 18 +++++------
 fs/proc/task_nommu.c | 20 ++++++-------
 lib/vsprintf.c       | 21 ++++++-------
 net/ipv4/fib_trie.c  | 30 ++++++++-----------
 net/ipv4/ping.c      | 19 ++++++------
 net/ipv4/tcp_ipv4.c  | 84 +++++++++++++++++++++++++---------------------------
 net/ipv4/udp.c       | 19 ++++++------
 net/phonet/socket.c  | 32 ++++++++++----------
 net/sctp/objcnt.c    |  5 ++--
 11 files changed, 132 insertions(+), 138 deletions(-)

diff --git a/fs/proc/consoles.c b/fs/proc/consoles.c
index b701eaa..42f2bb7 100644
--- a/fs/proc/consoles.c
+++ b/fs/proc/consoles.c
@@ -47,7 +47,7 @@ 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 = seq_printf(m, "%s%d", con->name, con->index);
 	len = 21 - len;
 	if (len < 1)
 		len = 1;
diff --git a/fs/proc/nommu.c b/fs/proc/nommu.c
index ccfd99b..91cfd19 100644
--- a/fs/proc/nommu.c
+++ b/fs/proc/nommu.c
@@ -50,16 +50,16 @@ static int nommu_region_show(struct seq_file *m, struct vm_region *region)
 		ino = inode->i_ino;
 	}
 
-	seq_printf(m,
-		   "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu %n",
-		   region->vm_start,
-		   region->vm_end,
-		   flags & VM_READ ? 'r' : '-',
-		   flags & VM_WRITE ? 'w' : '-',
-		   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);
+	len = seq_printf(m, "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu ",
+			 region->vm_start,
+			 region->vm_end,
+			 flags & VM_READ ? 'r' : '-',
+			 flags & VM_WRITE ? 'w' : '-',
+			 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);
 
 	if (file) {
 		len = 25 + sizeof(void *) * 6 - len;
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 107d026..f84ee9f 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -286,15 +286,15 @@ 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",
-			start,
-			end,
-			flags & VM_READ ? 'r' : '-',
-			flags & VM_WRITE ? 'w' : '-',
-			flags & VM_EXEC ? 'x' : '-',
-			flags & VM_MAYSHARE ? 's' : 'p',
-			pgoff,
-			MAJOR(dev), MINOR(dev), ino, &len);
+	len = seq_printf(m, "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu ",
+			 start,
+			 end,
+			 flags & VM_READ ? 'r' : '-',
+			 flags & VM_WRITE ? 'w' : '-',
+			 flags & VM_EXEC ? 'x' : '-',
+			 flags & VM_MAYSHARE ? 's' : 'p',
+			 pgoff,
+			 MAJOR(dev), MINOR(dev), ino);
 
 	/*
 	 * 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..1d7bbe5 100644
--- a/fs/proc/task_nommu.c
+++ b/fs/proc/task_nommu.c
@@ -155,16 +155,16 @@ static int nommu_vma_show(struct seq_file *m, struct vm_area_struct *vma,
 		pgoff = (loff_t)vma->vm_pgoff << PAGE_SHIFT;
 	}
 
-	seq_printf(m,
-		   "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu %n",
-		   vma->vm_start,
-		   vma->vm_end,
-		   flags & VM_READ ? 'r' : '-',
-		   flags & VM_WRITE ? 'w' : '-',
-		   flags & VM_EXEC ? 'x' : '-',
-		   flags & VM_MAYSHARE ? flags & VM_SHARED ? 'S' : 's' : 'p',
-		   pgoff,
-		   MAJOR(dev), MINOR(dev), ino, &len);
+	len = seq_printf(m, "%08lx-%08lx %c%c%c%c %08llx %02x:%02x %lu ",
+			 vma->vm_start,
+			 vma->vm_end,
+			 flags & VM_READ ? 'r' : '-',
+			 flags & VM_WRITE ? 'w' : '-',
+			 flags & VM_EXEC ? 'x' : '-',
+			 flags & VM_MAYSHARE ?
+			 flags & VM_SHARED ? 'S' : 's' : 'p',
+			 pgoff,
+			 MAJOR(dev), MINOR(dev), ino);
 
 	if (file) {
 		pad_len_spaces(m, len);
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 26559bd..43c2ea0 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1683,18 +1683,19 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 			break;
 
 		case FORMAT_TYPE_NRCHARS: {
+			/* skip %n 's argument */
 			u8 qualifier = spec.qualifier;
+			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 use of %%n in '%s'\n",
+				  old_fmt);
+
+			if (qualifier == 'l')
+				skip_arg = va_arg(args, long *);
+			else if (_tolower(qualifier) == 'z')
+				skip_arg = va_arg(args, size_t *);
+			else
+				skip_arg = va_arg(args, int *);
 			break;
 		}
 
diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
index 3df6d3e..ddf1e9c 100644
--- a/net/ipv4/fib_trie.c
+++ b/net/ipv4/fib_trie.c
@@ -2537,24 +2537,20 @@ static int fib_route_seq_show(struct seq_file *seq, void *v)
 				continue;
 
 			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",
-					 fi->fib_dev ? fi->fib_dev->name : "*",
-					 prefix,
-					 fi->fib_nh->nh_gw, flags, 0, 0,
-					 fi->fib_priority,
-					 mask,
-					 (fi->fib_advmss ?
-					  fi->fib_advmss + 40 : 0),
-					 fi->fib_window,
-					 fi->fib_rtt >> 3, &len);
+				len = seq_printf(seq, "%s\t%08X\t%08X\t%04X\t%d\t%u\t%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, fi->fib_priority, mask,
+						 (fi->fib_advmss ?
+						  fi->fib_advmss + 40 : 0),
+						 fi->fib_window,
+						 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",
-					 prefix, 0, flags, 0, 0, 0,
-					 mask, 0, 0, 0, &len);
+				len = seq_printf(seq, "*\t%08X\t%08X\t%04X\t%d\t%u\t%d\t%08X\t%d\t%u\t%u",
+						 prefix, 0, flags, 0, 0, 0,
+						 mask, 0, 0, 0);
 
 			seq_printf(seq, "%*s\n", 127 - len, "");
 		}
diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index d7d9882..ac8d79f 100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -1081,16 +1081,15 @@ static void ping_v4_format_sock(struct sock *sp, struct seq_file *f,
 	__u16 destp = ntohs(inet->inet_dport);
 	__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",
-		bucket, src, srcp, dest, destp, sp->sk_state,
-		sk_wmem_alloc_get(sp),
-		sk_rmem_alloc_get(sp),
-		0, 0L, 0,
-		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);
+	*len = seq_printf(f, "%5d: %08X:%04X %08X:%04X %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),
+			  0, 0L, 0,
+			  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));
 }
 
 static int ping_v4_seq_show(struct seq_file *seq, void *v)
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index b14266b..1279c16 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -2603,24 +2603,24 @@ static void get_openreq4(const struct sock *sk, const struct request_sock *req,
 	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",
-		i,
-		ireq->loc_addr,
-		ntohs(inet_sk(sk)->inet_sport),
-		ireq->rmt_addr,
-		ntohs(ireq->rmt_port),
-		TCP_SYN_RECV,
-		0, 0, /* could print option size, but that is af dependent. */
-		1,    /* timers active (only the expire timer) */
-		jiffies_delta_to_clock_t(delta),
-		req->num_timeout,
-		from_kuid_munged(seq_user_ns(f), uid),
-		0,  /* non standard timer */
-		0, /* open_requests have no inode */
-		atomic_read(&sk->sk_refcnt),
-		req,
-		len);
+	*len = seq_printf(f, "%4d: %08X:%04X %08X:%04X %02X %08X:%08X %02X:%08lX %08X %5u %8d %u %d %pK",
+			  i,
+			  ireq->loc_addr,
+			  ntohs(inet_sk(sk)->inet_sport),
+			  ireq->rmt_addr,
+			  ntohs(ireq->rmt_port),
+			  TCP_SYN_RECV,
+			  0, 0,	/* could print option size,
+				 * but that is af dependent.
+				 */
+			  1,	/* timers active (only the expire timer) */
+			  jiffies_delta_to_clock_t(delta),
+			  req->num_timeout,
+			  from_kuid_munged(seq_user_ns(f), uid),
+			  0,	/* non standard timer */
+			  0,	/* open_requests have no inode */
+			  atomic_read(&sk->sk_refcnt),
+			  req);
 }
 
 static void get_tcp4_sock(struct sock *sk, struct seq_file *f, int i, int *len)
@@ -2661,26 +2661,25 @@ 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",
-		i, src, srcp, dest, destp, sk->sk_state,
-		tp->write_seq - tp->snd_una,
-		rx_queue,
-		timer_active,
-		jiffies_delta_to_clock_t(timer_expires - jiffies),
-		icsk->icsk_retransmits,
-		from_kuid_munged(seq_user_ns(f), sock_i_uid(sk)),
-		icsk->icsk_probes_out,
-		sock_i_ino(sk),
-		atomic_read(&sk->sk_refcnt), sk,
-		jiffies_to_clock_t(icsk->icsk_rto),
-		jiffies_to_clock_t(icsk->icsk_ack.ato),
-		(icsk->icsk_ack.quick << 1) | icsk->icsk_ack.pingpong,
-		tp->snd_cwnd,
-		sk->sk_state == TCP_LISTEN ?
-		    (fastopenq ? fastopenq->max_qlen : 0) :
-		    (tcp_in_initial_slowstart(tp) ? -1 : tp->snd_ssthresh),
-		len);
+	*len = seq_printf(f, "%4d: %08X:%04X %08X:%04X %02X %08X:%08X %02X:%08lX %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,
+			  timer_active,
+			  jiffies_delta_to_clock_t(timer_expires - jiffies),
+			  icsk->icsk_retransmits,
+			  from_kuid_munged(seq_user_ns(f), sock_i_uid(sk)),
+			  icsk->icsk_probes_out,
+			  sock_i_ino(sk),
+			  atomic_read(&sk->sk_refcnt), sk,
+			  jiffies_to_clock_t(icsk->icsk_rto),
+			  jiffies_to_clock_t(icsk->icsk_ack.ato),
+			  (icsk->icsk_ack.quick << 1) | icsk->icsk_ack.pingpong,
+			  tp->snd_cwnd,
+			  sk->sk_state == TCP_LISTEN ?
+			  (fastopenq ? fastopenq->max_qlen : 0) :
+			  (tcp_in_initial_slowstart(tp) ? -1 :
+			   tp->snd_ssthresh));
 }
 
 static void get_timewait4_sock(const struct inet_timewait_sock *tw,
@@ -2695,11 +2694,10 @@ static void get_timewait4_sock(const struct inet_timewait_sock *tw,
 	destp = ntohs(tw->tw_dport);
 	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",
-		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);
+	*len = seq_printf(f, "%4d: %08X:%04X %08X:%04X %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);
 }
 
 #define TMPSZ 150
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 74d2c95..ddc24a2d 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -2158,16 +2158,15 @@ static void udp4_format_sock(struct sock *sp, struct seq_file *f,
 	__u16 destp	  = ntohs(inet->inet_dport);
 	__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",
-		bucket, src, srcp, dest, destp, sp->sk_state,
-		sk_wmem_alloc_get(sp),
-		sk_rmem_alloc_get(sp),
-		0, 0L, 0,
-		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);
+	*len = seq_printf(f, "%5d: %08X:%04X %08X:%04X %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),
+			  0, 0L, 0,
+			  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));
 }
 
 int udp4_seq_show(struct seq_file *seq, void *v)
diff --git a/net/phonet/socket.c b/net/phonet/socket.c
index 77e38f7..553f896 100644
--- a/net/phonet/socket.c
+++ b/net/phonet/socket.c
@@ -598,21 +598,20 @@ static int pn_sock_seq_show(struct seq_file *seq, void *v)
 	int len;
 
 	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);
+		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",
-			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);
+		len = seq_printf(seq, "%2d %04X:%04X:%02X %02X %08X:%08X %5d %lu %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));
 	}
 	seq_printf(seq, "%*s\n", 127 - len, "");
 	return 0;
@@ -788,15 +787,16 @@ static int pn_res_seq_show(struct seq_file *seq, void *v)
 	int len;
 
 	if (v == SEQ_START_TOKEN)
-		seq_printf(seq, "%s%n", "rs   uid inode", &len);
+		len = seq_puts(seq, "rs   uid inode");
 	else {
 		struct sock **psk = v;
 		struct sock *sk = *psk;
 
-		seq_printf(seq, "%02X %5u %lu%n",
-			   (int) (psk - pnres.sk),
-			   from_kuid_munged(seq_user_ns(seq), sock_i_uid(sk)),
-			   sock_i_ino(sk), &len);
+		len = 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));
 	}
 	seq_printf(seq, "%*s\n", 63 - len, "");
 	return 0;
diff --git a/net/sctp/objcnt.c b/net/sctp/objcnt.c
index 5ea573b..8034325 100644
--- a/net/sctp/objcnt.c
+++ b/net/sctp/objcnt.c
@@ -82,8 +82,9 @@ 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_printf(seq, "%s: %d",
+			 sctp_dbg_objcnt[i].label,
+			 atomic_read(sctp_dbg_objcnt[i].counter));
 	seq_printf(seq, "%*s\n", 127 - len, "");
 	return 0;
 }
-- 
1.8.1.2.459.gbcd45b4.dirty



_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel




[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux