* [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: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: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: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).