[PATCH] staging: dgnc: fix potential format string flaw

[PATCH] staging: dgnc: fix potential format string flaw

[Kees Cook]

Make sure that format strings cannot leak into printk() calls from the
msgbuf string.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/staging/dgnc/dgnc_driver.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_driver.c b/drivers/staging/dgnc/dgnc_driver.c
index f8c1e22..71d2b83 100644
--- a/drivers/staging/dgnc/dgnc_driver.c
+++ b/drivers/staging/dgnc/dgnc_driver.c
@@ -454,7 +454,7 @@ static void dgnc_cleanup_board(struct board_t *brd)
 
 		DGNC_LOCK(dgnc_global_lock, flags);
 		brd->msgbuf = NULL;
-		printk(brd->msgbuf_head);
+		printk("%s", brd->msgbuf_head);
 		kfree(brd->msgbuf_head);
 		brd->msgbuf_head = NULL;
 		DGNC_UNLOCK(dgnc_global_lock, flags);
@@ -710,7 +710,7 @@ static int dgnc_found_board(struct pci_dev *pdev, int id)
 	DPR_INIT(("dgnc_scan(%d) - printing out the msgbuf\n", i));
 	DGNC_LOCK(dgnc_global_lock, flags);
 	brd->msgbuf = NULL;
-	printk(brd->msgbuf_head);
+	printk("%s", brd->msgbuf_head);
 	kfree(brd->msgbuf_head);
 	brd->msgbuf_head = NULL;
 	DGNC_UNLOCK(dgnc_global_lock, flags);
-- 
1.7.9.5


-- 
Kees Cook
Chrome OS Security

Re: [PATCH] staging: dgnc: fix potential format string flaw

[Joe Perches]

On Tue, 2013-09-10 at 21:41 -0700, Kees Cook wrote:
> Make sure that format strings cannot leak into printk() calls from the
> msgbuf string.

	printf(string);
vs
	printf("%s", string);

How does this help?

Re: [PATCH] staging: dgnc: fix potential format string flaw

[Kees Cook]

On Tue, Sep 10, 2013 at 10:00 PM, Joe Perches <joe@perches.com> wrote:
> On Tue, 2013-09-10 at 21:41 -0700, Kees Cook wrote:
>> Make sure that format strings cannot leak into printk() calls from the
>> msgbuf string.
>
>         printf(string);
> vs
>         printf("%s", string);
>
> How does this help?

In the former case, format characters will get processed by the
sprintf logic. In the latter, they are printed as-is. In this specific
case, if there was a way to inject strings like "ohai %n" into the
msgbuf string, the former would actually attempt to resolve the %n. In
the simple case, this could lead to Oopses, and in the unlucky case,
it could allow arbitrary memory writing and execution control.

http://en.wikipedia.org/wiki/Uncontrolled_format_string

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH] staging: dgnc: fix potential format string flaw

[Dan Carpenter]

On Tue, Sep 10, 2013 at 10:19:17PM -0700, Kees Cook wrote:
> In the former case, format characters will get processed by the
> sprintf logic. In the latter, they are printed as-is. In this specific
> case, if there was a way to inject strings like "ohai %n" into the
> msgbuf string, the former would actually attempt to resolve the %n. In
> the simple case, this could lead to Oopses, and in the unlucky case,
> it could allow arbitrary memory writing and execution control.
> 
> http://en.wikipedia.org/wiki/Uncontrolled_format_string

The kernel ignores %n so hopefully it can't actually write to memory.

regards,
dan carpenter

Re: [PATCH] staging: dgnc: fix potential format string flaw

[Kees Cook]

On Wed, Sep 11, 2013 at 2:31 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Tue, Sep 10, 2013 at 10:19:17PM -0700, Kees Cook wrote:
>> In the former case, format characters will get processed by the
>> sprintf logic. In the latter, they are printed as-is. In this specific
>> case, if there was a way to inject strings like "ohai %n" into the
>> msgbuf string, the former would actually attempt to resolve the %n. In
>> the simple case, this could lead to Oopses, and in the unlucky case,
>> it could allow arbitrary memory writing and execution control.
>>
>> http://en.wikipedia.org/wiki/Uncontrolled_format_string
>
> The kernel ignores %n so hopefully it can't actually write to memory.

I wish! This is not the case, though. See FORMAT_TYPE_NRCHARS in
lib/vsprintf.c's vsnprintf().

$ git grep '%n' | wc -l
111

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH] staging: dgnc: fix potential format string flaw

[Joe Perches]

On Wed, 2013-09-11 at 11:19 -0700, Kees Cook wrote:
> On Wed, Sep 11, 2013 at 2:31 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Tue, Sep 10, 2013 at 10:19:17PM -0700, Kees Cook wrote:
> >> In the former case, format characters will get processed by the
> >> sprintf logic. In the latter, they are printed as-is. In this specific
> >> case, if there was a way to inject strings like "ohai %n" into the
> >> msgbuf string, the former would actually attempt to resolve the %n. In
> >> the simple case, this could lead to Oopses, and in the unlucky case,
> >> it could allow arbitrary memory writing and execution control.
> >>
> >> http://en.wikipedia.org/wiki/Uncontrolled_format_string
> >
> > The kernel ignores %n so hopefully it can't actually write to memory.
> 
> I wish! This is not the case, though. See FORMAT_TYPE_NRCHARS in
> lib/vsprintf.c's vsnprintf().
> 
> $ git grep '%n' | wc -l
> 111

Umm.

See: lib/vsprintf.c

/**
 * vsnprintf - Format a string and place it in a buffer
[...]
 * %n is ignored

%n does work for vsscanf though.

Re: [PATCH] staging: dgnc: fix potential format string flaw

[Kees Cook]

On Wed, Sep 11, 2013 at 12:09 PM, Joe Perches <joe@perches.com> wrote:
> On Wed, 2013-09-11 at 11:19 -0700, Kees Cook wrote:
>> On Wed, Sep 11, 2013 at 2:31 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> > On Tue, Sep 10, 2013 at 10:19:17PM -0700, Kees Cook wrote:
>> >> In the former case, format characters will get processed by the
>> >> sprintf logic. In the latter, they are printed as-is. In this specific
>> >> case, if there was a way to inject strings like "ohai %n" into the
>> >> msgbuf string, the former would actually attempt to resolve the %n. In
>> >> the simple case, this could lead to Oopses, and in the unlucky case,
>> >> it could allow arbitrary memory writing and execution control.
>> >>
>> >> http://en.wikipedia.org/wiki/Uncontrolled_format_string
>> >
>> > The kernel ignores %n so hopefully it can't actually write to memory.
>>
>> I wish! This is not the case, though. See FORMAT_TYPE_NRCHARS in
>> lib/vsprintf.c's vsnprintf().
>>
>> $ git grep '%n' | wc -l
>> 111
>
> Umm.
>
> See: lib/vsprintf.c
>
> /**
>  * vsnprintf - Format a string and place it in a buffer
> [...]
>  * %n is ignored
>
> %n does work for vsscanf though.

The comment is a lie:

        int len = 0;
        printk("len:%d\n", len);
        printk("%s%n\n", "Ohai!", &len);
        printk("len:%d\n", len);

[    0.025930] len:0
[    0.026003] Ohai!
[    0.026261] len:5

The functionality between scanf and printf was, I think, merged in
2009, if I'm reading the git blame correctly.

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH] staging: dgnc: fix potential format string flaw

[Joe Perches]

On Wed, 2013-09-11 at 12:25 -0700, Kees Cook wrote:
> On Wed, Sep 11, 2013 at 12:09 PM, Joe Perches <joe@perches.com> wrote:
> > On Wed, 2013-09-11 at 11:19 -0700, Kees Cook wrote:
> >> On Wed, Sep 11, 2013 at 2:31 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >> > On Tue, Sep 10, 2013 at 10:19:17PM -0700, Kees Cook wrote:
> >> >> In the former case, format characters will get processed by the
> >> >> sprintf logic. In the latter, they are printed as-is. In this specific
> >> >> case, if there was a way to inject strings like "ohai %n" into the
> >> >> msgbuf string, the former would actually attempt to resolve the %n. In
> >> >> the simple case, this could lead to Oopses, and in the unlucky case,
> >> >> it could allow arbitrary memory writing and execution control.
> >> >>
> >> >> http://en.wikipedia.org/wiki/Uncontrolled_format_string
> >> >
> >> > The kernel ignores %n so hopefully it can't actually write to memory.
> >>
> >> I wish! This is not the case, though. See FORMAT_TYPE_NRCHARS in
> >> lib/vsprintf.c's vsnprintf().
> >>
> >> $ git grep '%n' | wc -l
> >> 111
> >
> > Umm.
> >
> > See: lib/vsprintf.c
> >
> > /**
> >  * vsnprintf - Format a string and place it in a buffer
> > [...]
> >  * %n is ignored
> >
> > %n does work for vsscanf though.
> 
> The comment is a lie:
> 
>         int len = 0;
>         printk("len:%d\n", len);
>         printk("%s%n\n", "Ohai!", &len);
>         printk("len:%d\n", len);
> 
> [    0.025930] len:0
> [    0.026003] Ohai!
> [    0.026261] len:5
> 
> The functionality between scanf and printf was, I think, merged in
> 2009, if I'm reading the git blame correctly.

Yeah.

commit fef20d9c1380f04ba9492d6463148db07b413708
Author: Frederic Weisbecker <fweisbec@gmail.com>
Date:   Fri Mar 6 17:21:50 2009 +0100

    vsprintf: unify the format decoding layer for its 3 users

Maybe it should be reignored...

There are a few more in net/ though that may be pretty
easy to change to use the seq_printf return value.


net/ipv4/fib_trie.c-                            seq_printf(seq,
net/ipv4/fib_trie.c-                                     "%s\t%08X\t%08X\t%04X\t%d\t%u\t"
net/ipv4/fib_trie.c:                                     "%d\t%08X\t%d\t%u\t%u%n",
net/ipv4/fib_trie.c-                                     fi->fib_dev ? fi->fib_dev->name : "*",
--
net/ipv4/fib_trie.c-                            seq_printf(seq,
net/ipv4/fib_trie.c-                                     "*\t%08X\t%08X\t%04X\t%d\t%u\t"
net/ipv4/fib_trie.c:                                     "%d\t%08X\t%d\t%u\t%u%n",
net/ipv4/fib_trie.c-                                     prefix, 0, flags, 0, 0, 0,
--
net/ipv4/ping.c-        seq_printf(f, "%5d: %08X:%04X %08X:%04X"
net/ipv4/ping.c:                " %02X %08X:%08X %02X:%08lX %08X %5u %8d %lu %d %pK %d%n",
net/ipv4/ping.c-                bucket, src, srcp, dest, destp, sp->sk_state,
--
net/ipv4/tcp_ipv4.c-    seq_printf(f, "%4d: %08X:%04X %08X:%04X"
net/ipv4/tcp_ipv4.c:            " %02X %08X:%08X %02X:%08lX %08X %5u %8d %u %d %pK%n",
net/ipv4/tcp_ipv4.c-            i,
--
net/ipv4/tcp_ipv4.c-    seq_printf(f, "%4d: %08X:%04X %08X:%04X %02X %08X:%08X %02X:%08lX "
net/ipv4/tcp_ipv4.c:                    "%08X %5u %8d %lu %d %pK %lu %lu %u %u %d%n",
net/ipv4/tcp_ipv4.c-            i, src, srcp, dest, destp, sk->sk_state,
--
net/ipv4/tcp_ipv4.c-    seq_printf(f, "%4d: %08X:%04X %08X:%04X"
net/ipv4/tcp_ipv4.c:            " %02X %08X:%08X %02X:%08lX %08X %5d %8d %d %d %pK%n",
net/ipv4/tcp_ipv4.c-            i, src, srcp, dest, destp, tw->tw_substate, 0, 0,
--
net/ipv4/udp.c- seq_printf(f, "%5d: %08X:%04X %08X:%04X"
net/ipv4/udp.c:         " %02X %08X:%08X %02X:%08lX %08X %5u %8d %lu %d %pK %d%n",
net/ipv4/udp.c-         bucket, src, srcp, dest, destp, sp->sk_state,
--
net/sctp/objcnt.c:	seq_printf(seq, "%s: %d%n", sctp_dbg_objcnt[i].label,
net/sctp/objcnt.c-				atomic_read(sctp_dbg_objcnt[i].counter), &len);

Re: [PATCH] staging: dgnc: fix potential format string flaw

[Kees Cook]

On Wed, Sep 11, 2013 at 12:45 PM, Joe Perches <joe@perches.com> wrote:
> On Wed, 2013-09-11 at 12:25 -0700, Kees Cook wrote:
>> On Wed, Sep 11, 2013 at 12:09 PM, Joe Perches <joe@perches.com> wrote:
>> > On Wed, 2013-09-11 at 11:19 -0700, Kees Cook wrote:
>> >> On Wed, Sep 11, 2013 at 2:31 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> >> > On Tue, Sep 10, 2013 at 10:19:17PM -0700, Kees Cook wrote:
>> >> >> In the former case, format characters will get processed by the
>> >> >> sprintf logic. In the latter, they are printed as-is. In this specific
>> >> >> case, if there was a way to inject strings like "ohai %n" into the
>> >> >> msgbuf string, the former would actually attempt to resolve the %n. In
>> >> >> the simple case, this could lead to Oopses, and in the unlucky case,
>> >> >> it could allow arbitrary memory writing and execution control.
>> >> >>
>> >> >> http://en.wikipedia.org/wiki/Uncontrolled_format_string
>> >> >
>> >> > The kernel ignores %n so hopefully it can't actually write to memory.
>> >>
>> >> I wish! This is not the case, though. See FORMAT_TYPE_NRCHARS in
>> >> lib/vsprintf.c's vsnprintf().
>> >>
>> >> $ git grep '%n' | wc -l
>> >> 111
>> >
>> > Umm.
>> >
>> > See: lib/vsprintf.c
>> >
>> > /**
>> >  * vsnprintf - Format a string and place it in a buffer
>> > [...]
>> >  * %n is ignored
>> >
>> > %n does work for vsscanf though.
>>
>> The comment is a lie:
>>
>>         int len = 0;
>>         printk("len:%d\n", len);
>>         printk("%s%n\n", "Ohai!", &len);
>>         printk("len:%d\n", len);
>>
>> [    0.025930] len:0
>> [    0.026003] Ohai!
>> [    0.026261] len:5
>>
>> The functionality between scanf and printf was, I think, merged in
>> 2009, if I'm reading the git blame correctly.
>
> Yeah.
>
> commit fef20d9c1380f04ba9492d6463148db07b413708
> Author: Frederic Weisbecker <fweisbec@gmail.com>
> Date:   Fri Mar 6 17:21:50 2009 +0100
>
>     vsprintf: unify the format decoding layer for its 3 users
>
> Maybe it should be reignored...
>
> There are a few more in net/ though that may be pretty
> easy to change to use the seq_printf return value.

I would love to remove %n. It's not clear to me how to re-split it
from scanf, though. I'd need to study this code a bunch more. Dropping
%n from all its non-scanf uses would be a good first-step, though.

-Kees

>
>
> net/ipv4/fib_trie.c-                            seq_printf(seq,
> net/ipv4/fib_trie.c-                                     "%s\t%08X\t%08X\t%04X\t%d\t%u\t"
> net/ipv4/fib_trie.c:                                     "%d\t%08X\t%d\t%u\t%u%n",
> net/ipv4/fib_trie.c-                                     fi->fib_dev ? fi->fib_dev->name : "*",
> --
> net/ipv4/fib_trie.c-                            seq_printf(seq,
> net/ipv4/fib_trie.c-                                     "*\t%08X\t%08X\t%04X\t%d\t%u\t"
> net/ipv4/fib_trie.c:                                     "%d\t%08X\t%d\t%u\t%u%n",
> net/ipv4/fib_trie.c-                                     prefix, 0, flags, 0, 0, 0,
> --
> net/ipv4/ping.c-        seq_printf(f, "%5d: %08X:%04X %08X:%04X"
> net/ipv4/ping.c:                " %02X %08X:%08X %02X:%08lX %08X %5u %8d %lu %d %pK %d%n",
> net/ipv4/ping.c-                bucket, src, srcp, dest, destp, sp->sk_state,
> --
> net/ipv4/tcp_ipv4.c-    seq_printf(f, "%4d: %08X:%04X %08X:%04X"
> net/ipv4/tcp_ipv4.c:            " %02X %08X:%08X %02X:%08lX %08X %5u %8d %u %d %pK%n",
> net/ipv4/tcp_ipv4.c-            i,
> --
> net/ipv4/tcp_ipv4.c-    seq_printf(f, "%4d: %08X:%04X %08X:%04X %02X %08X:%08X %02X:%08lX "
> net/ipv4/tcp_ipv4.c:                    "%08X %5u %8d %lu %d %pK %lu %lu %u %u %d%n",
> net/ipv4/tcp_ipv4.c-            i, src, srcp, dest, destp, sk->sk_state,
> --
> net/ipv4/tcp_ipv4.c-    seq_printf(f, "%4d: %08X:%04X %08X:%04X"
> net/ipv4/tcp_ipv4.c:            " %02X %08X:%08X %02X:%08lX %08X %5d %8d %d %d %pK%n",
> net/ipv4/tcp_ipv4.c-            i, src, srcp, dest, destp, tw->tw_substate, 0, 0,
> --
> net/ipv4/udp.c- seq_printf(f, "%5d: %08X:%04X %08X:%04X"
> net/ipv4/udp.c:         " %02X %08X:%08X %02X:%08lX %08X %5u %8d %lu %d %pK %d%n",
> net/ipv4/udp.c-         bucket, src, srcp, dest, destp, sp->sk_state,
> --
> net/sctp/objcnt.c:      seq_printf(seq, "%s: %d%n", sctp_dbg_objcnt[i].label,
> net/sctp/objcnt.c-                              atomic_read(sctp_dbg_objcnt[i].counter), &len);
>
>
>



-- 
Kees Cook
Chrome OS Security

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

[Joe Perches]

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

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

[Kees Cook]

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

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

[Joe Perches]

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.

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

[Tetsuo Handa]

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);

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

[Joe Perches]

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);

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

[Al Viro]

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".

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

[Joe Perches]

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.

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

[David Laight]

> 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

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

[Joe Perches]

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 *);

Re: [PATCH] staging: dgnc: fix potential format string flaw

[Dan Carpenter]

>  * %n is ignored

Really it should trigger a WARN_ON_ONCE().  There is code like
show_console_dev() which relies on it to work.  If we ignore %n it
causes another bug.

regards,
dan carpenter

Re: [PATCH] staging: dgnc: fix potential format string flaw

[Dan Carpenter]

On Wed, Sep 11, 2013 at 11:19:11AM -0700, Kees Cook wrote:
> On Wed, Sep 11, 2013 at 2:31 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Tue, Sep 10, 2013 at 10:19:17PM -0700, Kees Cook wrote:
> >> In the former case, format characters will get processed by the
> >> sprintf logic. In the latter, they are printed as-is. In this specific
> >> case, if there was a way to inject strings like "ohai %n" into the
> >> msgbuf string, the former would actually attempt to resolve the %n. In
> >> the simple case, this could lead to Oopses, and in the unlucky case,
> >> it could allow arbitrary memory writing and execution control.
> >>
> >> http://en.wikipedia.org/wiki/Uncontrolled_format_string
> >
> > The kernel ignores %n so hopefully it can't actually write to memory.
> 
> I wish! This is not the case, though. See FORMAT_TYPE_NRCHARS in
> lib/vsprintf.c's vsnprintf().
> 
> $ git grep '%n' | wc -l
> 111
> 

Hm...  That's unfortunate.  The comments were shifted around so it says
it's ignored but it's not.

regards,
dan carpenter

Re: [PATCH] staging: dgnc: fix potential format string flaw

[Kees Cook]

On Wed, Sep 11, 2013 at 12:22 PM, Dan Carpenter
<dan.carpenter@oracle.com> wrote:
> On Wed, Sep 11, 2013 at 11:19:11AM -0700, Kees Cook wrote:
>> On Wed, Sep 11, 2013 at 2:31 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> > On Tue, Sep 10, 2013 at 10:19:17PM -0700, Kees Cook wrote:
>> >> In the former case, format characters will get processed by the
>> >> sprintf logic. In the latter, they are printed as-is. In this specific
>> >> case, if there was a way to inject strings like "ohai %n" into the
>> >> msgbuf string, the former would actually attempt to resolve the %n. In
>> >> the simple case, this could lead to Oopses, and in the unlucky case,
>> >> it could allow arbitrary memory writing and execution control.
>> >>
>> >> http://en.wikipedia.org/wiki/Uncontrolled_format_string
>> >
>> > The kernel ignores %n so hopefully it can't actually write to memory.
>>
>> I wish! This is not the case, though. See FORMAT_TYPE_NRCHARS in
>> lib/vsprintf.c's vsnprintf().
>>
>> $ git grep '%n' | wc -l
>> 111
>>
>
> Hm...  That's unfortunate.  The comments were shifted around so it says
> it's ignored but it's not.

Outside of scanf, there are very few uses, though:

$ git grep %n | grep print
...
net/phonet/socket.c:            seq_printf(seq, "%s%n", "pt  loc  rem
rs st tx_queue rx_queue "
net/phonet/socket.c:            seq_printf(seq, "%s%n", "rs   uid inode", &len);
net/phonet/socket.c:            seq_printf(seq, "%02X %5u %lu%n",
net/sctp/objcnt.c:      seq_printf(seq, "%s: %d%n", sctp_dbg_objcnt[i].label,

-Kees

-- 
Kees Cook
Chrome OS Security