* [RFC PATCH] vsnprintf: Remove use of %n and convert existing uses
[not found] ` <CAGXu5jLTjrPr=GwOmMcu55Qp9K-_XS75xq9NJ0vBUEwy-i_YMw@mail.gmail.com>
@ 2013-09-11 23:22 ` Joe Perches
2013-09-11 23:29 ` Kees Cook
2013-09-11 23:40 ` Tetsuo Handa
0 siblings, 2 replies; 9+ messages in thread
From: Joe Perches @ 2013-09-11 23:22 UTC (permalink / raw)
To: LKML
Cc: KOSAKI Motohiro, Kees Cook, Frederic Weisbecker, Dan Carpenter,
devel, Greg Kroah-Hartman, Tushar Behera, Lidza Louina,
David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, Remi Denis-Courmont,
Vlad Yasevich, Neil Horman, netdev, linux-sctp
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@perches.com>
Acked-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> (proc bits)
cc: Kees Cook <keescook@chromium.org>
cc: Frederic Weisbecker <fweisbec@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [RFC PATCH] vsnprintf: Remove use of %n and convert existing uses
2013-09-11 23:22 ` [RFC PATCH] vsnprintf: Remove use of %n and convert existing uses Joe Perches
@ 2013-09-11 23:29 ` Kees Cook
2013-09-11 23:43 ` Joe Perches
2013-09-11 23:40 ` Tetsuo Handa
1 sibling, 1 reply; 9+ messages in thread
From: Kees Cook @ 2013-09-11 23:29 UTC (permalink / raw)
To: Joe Perches
Cc: devel, Lidza Louina, Neil Horman, Patrick McHardy,
Hideaki YOSHIFUJI, Frederic Weisbecker, Vlad Yasevich, LKML,
James Morris, Remi Denis-Courmont, linux-sctp, netdev,
KOSAKI Motohiro, Greg Kroah-Hartman, Alexey Kuznetsov,
David S. Miller, Dan Carpenter, Tushar Behera
On Wed, Sep 11, 2013 at 4:22 PM, Joe Perches <joe@perches.com> wrote:
> 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@perches.com>
> Acked-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> (proc bits)
> cc: Kees Cook <keescook@chromium.org>
> cc: Frederic Weisbecker <fweisbec@gmail.com>
Yes, please. It might also be worth updating
Documentation/printk-formats.txt to mention that %n has intentionally
removed and will be ignored.
Reviewed-by: Kees Cook <keescook@chromium.org>
-Kees
>
> ---
>
> 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
>
>
>
--
Kees Cook
Chrome OS Security
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH] vsnprintf: Remove use of %n and convert existing uses
2013-09-11 23:22 ` [RFC PATCH] vsnprintf: Remove use of %n and convert existing uses Joe Perches
2013-09-11 23:29 ` Kees Cook
@ 2013-09-11 23:40 ` Tetsuo Handa
2013-09-12 0:04 ` Joe Perches
1 sibling, 1 reply; 9+ messages in thread
From: Tetsuo Handa @ 2013-09-11 23:40 UTC (permalink / raw)
To: joe, linux-kernel
Cc: kosaki.motohiro, keescook, fweisbec, dan.carpenter, devel, gregkh,
tushar.behera, lidza.louina, davem, kuznet, jmorris, yoshfuji,
kaber, courmisch, vyasevich, nhorman, netdev, linux-sctp
Joe Perches wrote:
> - seq_printf(m, "%s%d%n", con->name, con->index, &len);
> + len = seq_printf(m, "%s%d", con->name, con->index);
Isn't len always 0 or -1 ?
int seq_vprintf(struct seq_file *m, const char *f, va_list args)
{
int len;
if (m->count < m->size) {
len = vsnprintf(m->buf + m->count, m->size - m->count, f, args);
if (m->count + len < m->size) {
m->count += len;
return 0;
}
}
seq_set_overflow(m);
return -1;
}
EXPORT_SYMBOL(seq_vprintf);
int seq_printf(struct seq_file *m, const char *f, ...)
{
int ret;
va_list args;
va_start(args, f);
ret = seq_vprintf(m, f, args);
va_end(args);
return ret;
}
EXPORT_SYMBOL(seq_printf);
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH] vsnprintf: Remove use of %n and convert existing uses
2013-09-11 23:29 ` Kees Cook
@ 2013-09-11 23:43 ` Joe Perches
0 siblings, 0 replies; 9+ messages in thread
From: Joe Perches @ 2013-09-11 23:43 UTC (permalink / raw)
To: Kees Cook
Cc: LKML, KOSAKI Motohiro, Frederic Weisbecker, Dan Carpenter, devel,
Greg Kroah-Hartman, Tushar Behera, Lidza Louina, David S. Miller,
Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI,
Patrick McHardy, Remi Denis-Courmont, Vlad Yasevich, Neil Horman,
netdev, linux-sctp
On Wed, 2013-09-11 at 16:29 -0700, Kees Cook wrote:
> On Wed, Sep 11, 2013 at 4:22 PM, Joe Perches <joe@perches.com> wrote:
> > 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@perches.com>
> > Acked-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> (proc bits)
> > cc: Kees Cook <keescook@chromium.org>
> > cc: Frederic Weisbecker <fweisbec@gmail.com>
>
> Yes, please. It might also be worth updating
> Documentation/printk-formats.txt to mention that %n has intentionally
> removed and will be ignored.
Fine with me if you want to update that file.
It doesn't currently try to be a complete man page
for vsnprintf though.
vsprintf.c does have kernel-doc documentation and
that already does show that %n is ignored.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH] vsnprintf: Remove use of %n and convert existing uses
2013-09-11 23:40 ` Tetsuo Handa
@ 2013-09-12 0:04 ` Joe Perches
2013-09-12 0:19 ` Al Viro
0 siblings, 1 reply; 9+ messages in thread
From: Joe Perches @ 2013-09-12 0:04 UTC (permalink / raw)
To: Tetsuo Handa
Cc: linux-kernel, kosaki.motohiro, keescook, fweisbec, dan.carpenter,
devel, gregkh, tushar.behera, lidza.louina, davem, kuznet,
jmorris, yoshfuji, kaber, courmisch, vyasevich, nhorman, netdev,
linux-sctp
On Thu, 2013-09-12 at 08:40 +0900, Tetsuo Handa wrote:
> Joe Perches wrote:
> > - seq_printf(m, "%s%d%n", con->name, con->index, &len);
> > + len = seq_printf(m, "%s%d", con->name, con->index);
>
> Isn't len always 0 or -1 ?
Right. Well you're no fun...
These uses would seem broken anyway because the
seq_printf isn't itself tested for correctness.
Hmm.
Also, there's a large amount of code that appears
to do calculations with pos or len like:
pos += seq_printf(handle, fmt. ...)
There are very few that seem to use it correctly
like netfilter.
$ grep -rP --include=*.[ch] "^[ \t]*\S[ \t\S]*\bseq_[v]?printf\b" *
Suggestions?
> int seq_vprintf(struct seq_file *m, const char *f, va_list args)
> {
> int len;
>
> if (m->count < m->size) {
> len = vsnprintf(m->buf + m->count, m->size - m->count, f, args);
> if (m->count + len < m->size) {
> m->count += len;
> return 0;
> }
> }
> seq_set_overflow(m);
> return -1;
> }
> EXPORT_SYMBOL(seq_vprintf);
>
> int seq_printf(struct seq_file *m, const char *f, ...)
> {
> int ret;
> va_list args;
>
> va_start(args, f);
> ret = seq_vprintf(m, f, args);
> va_end(args);
>
> return ret;
> }
> EXPORT_SYMBOL(seq_printf);
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH] vsnprintf: Remove use of %n and convert existing uses
2013-09-12 0:04 ` Joe Perches
@ 2013-09-12 0:19 ` Al Viro
2013-09-12 0:41 ` Joe Perches
2013-09-12 8:06 ` David Laight
0 siblings, 2 replies; 9+ messages in thread
From: Al Viro @ 2013-09-12 0:19 UTC (permalink / raw)
To: Joe Perches
Cc: Tetsuo Handa, linux-kernel, kosaki.motohiro, keescook, fweisbec,
dan.carpenter, devel, gregkh, tushar.behera, lidza.louina, davem,
kuznet, jmorris, yoshfuji, kaber, courmisch, vyasevich, nhorman,
netdev, linux-sctp
On Wed, Sep 11, 2013 at 05:04:17PM -0700, Joe Perches wrote:
> On Thu, 2013-09-12 at 08:40 +0900, Tetsuo Handa wrote:
> > Joe Perches wrote:
> > > - seq_printf(m, "%s%d%n", con->name, con->index, &len);
> > > + len = seq_printf(m, "%s%d", con->name, con->index);
> >
> > Isn't len always 0 or -1 ?
>
> Right. Well you're no fun...
>
> These uses would seem broken anyway because the
> seq_printf isn't itself tested for correctness.
>
> Hmm.
>
> Also, there's a large amount of code that appears
> to do calculations with pos or len like:
>
> pos += seq_printf(handle, fmt. ...)
... and most of that code proceeds to ignore pos completely.
Note that ->show() is *NOT* supposed to return the number of
characters it has/would like to have produced. Just return
0 and be done with that; overflows are dealt with just fine.
The large amount, BTW, is below 100 lines, AFAICS, in rather
few files.
> There are very few that seem to use it correctly
> like netfilter.
> Suggestions?
Just bury the cargo-culting crap. All those += seq_printf() should
be simply calling it. The *only* reason to look at the return
value is "if we'd already overflown the buffer, I'd rather skipped
the costly generation of the rest of the record". In that case
seq_printf() returning -1 means "skip it, nothing else will fit and
caller will be repeating with bigger buffer anyway".
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH] vsnprintf: Remove use of %n and convert existing uses
2013-09-12 0:19 ` Al Viro
@ 2013-09-12 0:41 ` Joe Perches
2013-09-12 8:06 ` David Laight
1 sibling, 0 replies; 9+ messages in thread
From: Joe Perches @ 2013-09-12 0:41 UTC (permalink / raw)
To: Al Viro
Cc: Tetsuo Handa, linux-kernel, kosaki.motohiro, keescook, fweisbec,
dan.carpenter, devel, gregkh, tushar.behera, lidza.louina, davem,
kuznet, jmorris, yoshfuji, kaber, courmisch, vyasevich, nhorman,
netdev, linux-sctp
On Thu, 2013-09-12 at 01:19 +0100, Al Viro wrote:
> On Wed, Sep 11, 2013 at 05:04:17PM -0700, Joe Perches wrote:
> > On Thu, 2013-09-12 at 08:40 +0900, Tetsuo Handa wrote:
> > > Joe Perches wrote:
> > > > - seq_printf(m, "%s%d%n", con->name, con->index, &len);
> > > > + len = seq_printf(m, "%s%d", con->name, con->index);
> > >
> > > Isn't len always 0 or -1 ?
> >
> > Right. Well you're no fun...
> >
> > These uses would seem broken anyway because the
> > seq_printf isn't itself tested for correctness.
> >
> > Hmm.
> >
> > Also, there's a large amount of code that appears
> > to do calculations with pos or len like:
> >
> > pos += seq_printf(handle, fmt. ...)
>
> ... and most of that code proceeds to ignore pos completely.
> Note that ->show() is *NOT* supposed to return the number of
> characters it has/would like to have produced. Just return
> 0 and be done with that; overflows are dealt with just fine.
> The large amount, BTW, is below 100 lines, AFAICS, in rather
> few files.
Unfortunately, when you count the uses of
return seq_printf(...)
it's rather higher and all the callers need
to be chased down too.
$ grep -rP --include=*.[ch] "^[ \t]*(\S[ \t\S]*=|return[\s\(]*)\s*\bseq_[v]?printf\b" * | wc -l
320
$ grep -rPl --include=*.[ch] "^[ \t]*(\S[ \t\S]*=|return[\s\(]*)\s*\bseq_[v]?printf\b" *|wc -l
81
> Just bury the cargo-culting crap. All those += seq_printf() should
> be simply calling it.
Most likely.
> The *only* reason to look at the return
> value is "if we'd already overflown the buffer, I'd rather skipped
> the costly generation of the rest of the record". In that case
> seq_printf() returning -1 means "skip it, nothing else will fit and
> caller will be repeating with bigger buffer anyway".
Perhaps changing the seq_vprintf return from 0 to len
and testing for -1 would work.
Still would need to change a few lines in netfilter
and probably a few other places.
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [RFC PATCH] vsnprintf: Remove use of %n and convert existing uses
2013-09-12 0:19 ` Al Viro
2013-09-12 0:41 ` Joe Perches
@ 2013-09-12 8:06 ` David Laight
2013-09-12 15:59 ` Joe Perches
1 sibling, 1 reply; 9+ messages in thread
From: David Laight @ 2013-09-12 8:06 UTC (permalink / raw)
To: Al Viro, Joe Perches
Cc: Tetsuo Handa, linux-kernel, kosaki.motohiro, keescook, fweisbec,
dan.carpenter, devel, gregkh, tushar.behera, lidza.louina, davem,
kuznet, jmorris, yoshfuji, kaber, courmisch, vyasevich, nhorman,
netdev, linux-sctp
> On Wed, Sep 11, 2013 at 05:04:17PM -0700, Joe Perches wrote:
> > On Thu, 2013-09-12 at 08:40 +0900, Tetsuo Handa wrote:
> > > Joe Perches wrote:
> > > > - seq_printf(m, "%s%d%n", con->name, con->index, &len);
> > > > + len = seq_printf(m, "%s%d", con->name, con->index);
> > >
> > > Isn't len always 0 or -1 ?
> >
> > Right. Well you're no fun...
> >
> > These uses would seem broken anyway because the
> > seq_printf isn't itself tested for correctness.
> >
> > Hmm.
> >
> > Also, there's a large amount of code that appears
> > to do calculations with pos or len like:
> >
> > pos += seq_printf(handle, fmt. ...)
>
> ... and most of that code proceeds to ignore pos completely.
> Note that ->show() is *NOT* supposed to return the number of
> characters it has/would like to have produced. Just return
> 0 and be done with that; overflows are dealt with just fine.
> The large amount, BTW, is below 100 lines, AFAICS, in rather
> few files.
>
> > There are very few that seem to use it correctly
> > like netfilter.
>
> > Suggestions?
Change the return type of seq_printf() to void and require that
code use access functions/macros to find the length and error
status. Possibly save the length of the last call separately.
David
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH] vsnprintf: Remove use of %n and convert existing uses
2013-09-12 8:06 ` David Laight
@ 2013-09-12 15:59 ` Joe Perches
0 siblings, 0 replies; 9+ messages in thread
From: Joe Perches @ 2013-09-12 15:59 UTC (permalink / raw)
To: David Laight
Cc: Al Viro, Tetsuo Handa, linux-kernel, kosaki.motohiro, keescook,
fweisbec, dan.carpenter, devel, gregkh, tushar.behera,
lidza.louina, davem, kuznet, jmorris, yoshfuji, kaber, courmisch,
vyasevich, nhorman, netdev, linux-sctp
On Thu, 2013-09-12 at 09:06 +0100, David Laight wrote:
> > On Wed, Sep 11, 2013 at 05:04:17PM -0700, Joe Perches wrote:
> > > On Thu, 2013-09-12 at 08:40 +0900, Tetsuo Handa wrote:
> > > > Joe Perches wrote:
> > > > > - seq_printf(m, "%s%d%n", con->name, con->index, &len);
> > > > > + len = seq_printf(m, "%s%d", con->name, con->index);
> > > >
> > > > Isn't len always 0 or -1 ?
> > >
> > > Right. Well you're no fun...
[]
> > > Suggestions?
>
> Change the return type of seq_printf() to void and require that
> code use access functions/macros to find the length and error
> status. Possibly save the length of the last call separately.
Are there any races if this is done?
---
fs/seq_file.c | 15 +++++++--------
include/linux/seq_file.h | 6 ++++--
2 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/fs/seq_file.c b/fs/seq_file.c
index 3135c25..b6c9eab 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -389,32 +389,31 @@ int seq_escape(struct seq_file *m, const char *s, const char *esc)
}
EXPORT_SYMBOL(seq_escape);
-int seq_vprintf(struct seq_file *m, const char *f, va_list args)
+void seq_vprintf(struct seq_file *m, const char *f, va_list args)
{
int len;
if (m->count < m->size) {
len = vsnprintf(m->buf + m->count, m->size - m->count, f, args);
+ m->last_len = len;
if (m->count + len < m->size) {
m->count += len;
- return 0;
+ m->last_rtn = 0;
+ return;
}
}
seq_set_overflow(m);
- return -1;
+ m->last_rtn = -1;
}
EXPORT_SYMBOL(seq_vprintf);
-int seq_printf(struct seq_file *m, const char *f, ...)
+void seq_printf(struct seq_file *m, const char *f, ...)
{
- int ret;
va_list args;
va_start(args, f);
- ret = seq_vprintf(m, f, args);
+ seq_vprintf(m, f, args);
va_end(args);
-
- return ret;
}
EXPORT_SYMBOL(seq_printf);
diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
index 4e32edc..9af05e1 100644
--- a/include/linux/seq_file.h
+++ b/include/linux/seq_file.h
@@ -20,6 +20,8 @@ struct seq_file {
size_t size;
size_t from;
size_t count;
+ size_t last_len;
+ int last_rtn;
loff_t index;
loff_t read_pos;
u64 version;
@@ -89,8 +91,8 @@ int seq_putc(struct seq_file *m, char c);
int seq_puts(struct seq_file *m, const char *s);
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) void seq_printf(struct seq_file *, const char *, ...);
+__printf(2, 0) void seq_vprintf(struct seq_file *, const char *, va_list args);
int seq_path(struct seq_file *, const struct path *, const char *);
int seq_dentry(struct seq_file *, struct dentry *, const char *);
^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-09-12 15:59 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20130911044116.GA17294@www.outflux.net>
[not found] ` <1378875632.606.5.camel@joe-AO722>
[not found] ` <CAGXu5jLm1RNp4mB2KfeRumTqeWgcAsZ4CpQQQGA67WukERefXQ@mail.gmail.com>
[not found] ` <20130911093118.GD25896@mwanda>
[not found] ` <CAGXu5j+H_RLwj9c6CKxMpaYeR9kS-qDnK+e1mOHHt9zh6vAm=w@mail.gmail.com>
[not found] ` <1378926562.4714.11.camel@joe-AO722>
[not found] ` <CAGXu5jLO6109fkejXqX=WhkQnfAUeWtTcow0v1BNqyi=8=Qa3Q@mail.gmail.com>
[not found] ` <1378928700.4714.17.camel@joe-AO722>
[not found] ` <CAGXu5jLTjrPr=GwOmMcu55Qp9K-_XS75xq9NJ0vBUEwy-i_YMw@mail.gmail.com>
2013-09-11 23:22 ` [RFC PATCH] vsnprintf: Remove use of %n and convert existing uses Joe Perches
2013-09-11 23:29 ` Kees Cook
2013-09-11 23:43 ` Joe Perches
2013-09-11 23:40 ` Tetsuo Handa
2013-09-12 0:04 ` Joe Perches
2013-09-12 0:19 ` Al Viro
2013-09-12 0:41 ` Joe Perches
2013-09-12 8:06 ` David Laight
2013-09-12 15:59 ` Joe Perches
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).