Hi,
This patch removes support for the %n format specifier from
vsprintf, remove the sole use of %n in the kernel, fixes seq_printf to
return the number of characters written rather than 0 on success
(thus getting rid of %n), and fixes the only two callers of seq_printf
who bothered to look at the return value.
The printf man page has this to say about '%n':
"The number of characters written so far is stored into the integer
indicated by the int * (or variant) pointer argument. No argument is
converted."
Very little code actually uses %n for that. These days, %n has a much
more common use - in printf format string exploits. Since there's only
one user of %n in the kernel, this patch removes support for it and
fixes the user. To preempt the obvious argument, I agree that printk
should look and behave as much as possible as printf - except where
it's harmful. We don't support floating point, for example, and I
doubt we should support %n - although I don't strongly care one way or
another.
The patch is a bit more intrusive than I would've
liked. fs/proc/task_mmu.c:show_map() called seq_printf with %n, in
order to calculate the proper number of spaces to add in each line in
the /proc/$PID/map output for named maps. I changed seq_printf to
return the number of characters written (not including the final \0)
as returned from vcnsprintf. This required two changes to the only two
callers of seq_printf who bothered to check the return value. One of
them (cris/arch-v10/kernel/setup.c) was trivial, and the other
(arch/i386/kernel/cpu/mtrr/if.c) didn't do anything with the return
value anyway.
Patch is against 2.6.5-rc3, compiles and tested. Please apply if you
consider it appropriate..
diff -Naurb -X /home/muli/w/dontdiff linux-2.5/arch/cris/arch-v10/kernel/setup.c 2.6.5-rc3-pn/arch/cris/arch-v10/kernel/setup.c
--- linux-2.5/arch/cris/arch-v10/kernel/setup.c 2003-07-10 20:33:07.000000000 +0300
+++ 2.6.5-rc3-pn/arch/cris/arch-v10/kernel/setup.c 2004-04-06 11:09:15.000000000 +0200
@@ -52,6 +52,7 @@
{
unsigned long revision;
struct cpu_info *info;
+ int i;
/* read the version register in the CPU and print some stuff */
@@ -62,7 +63,7 @@
else
info = &cpu_info[revision];
- return seq_printf(m,
+ i = seq_printf(m,
"processor\t: 0\n"
"cpu\t\t: CRIS\n"
"cpu revision\t: %lu\n"
@@ -91,6 +92,7 @@
info->flags & HAS_USB ? "yes" : "no",
(loops_per_jiffy * HZ + 500) / 500000,
((loops_per_jiffy * HZ + 500) / 5000) % 100);
+ return (i < 0) ? i : 0;
}
#endif /* CONFIG_PROC_FS */
diff -Naurb -X /home/muli/w/dontdiff linux-2.5/arch/i386/kernel/cpu/mtrr/if.c 2.6.5-rc3-pn/arch/i386/kernel/cpu/mtrr/if.c
--- linux-2.5/arch/i386/kernel/cpu/mtrr/if.c 2004-01-31 19:45:38.000000000 +0200
+++ 2.6.5-rc3-pn/arch/i386/kernel/cpu/mtrr/if.c 2004-04-06 11:10:08.000000000 +0200
@@ -320,12 +320,11 @@
static int mtrr_seq_show(struct seq_file *seq, void *offset)
{
char factor;
- int i, max, len;
+ int i, max;
mtrr_type type;
unsigned long base;
unsigned int size;
- len = 0;
max = num_var_ranges;
for (i = 0; i < max; i++) {
mtrr_if->get(i, &base, &size, &type);
@@ -341,7 +340,7 @@
size >>= 20 - PAGE_SHIFT;
}
/* RED-PEN: base can be > 32bit */
- len += seq_printf(seq,
+ seq_printf(seq,
"reg%02i: base=0x%05lx000 (%4liMB), size=%4i%cB: %s, count=%d\n",
i, base, base >> (20 - PAGE_SHIFT), size, factor,
mtrr_attrib_to_str(type), usage_table[i]);
diff -Naurb -X /home/muli/w/dontdiff linux-2.5/fs/proc/task_mmu.c 2.6.5-rc3-pn/fs/proc/task_mmu.c
--- linux-2.5/fs/proc/task_mmu.c 2004-01-20 19:20:25.000000000 +0200
+++ 2.6.5-rc3-pn/fs/proc/task_mmu.c 2004-04-06 11:11:33.000000000 +0200
@@ -91,7 +91,7 @@
ino = inode->i_ino;
}
- seq_printf(m, "%08lx-%08lx %c%c%c%c %08lx %02x:%02x %lu %n",
+ len = seq_printf(m, "%08lx-%08lx %c%c%c%c %08lx %02x:%02x %lu ",
map->vm_start,
map->vm_end,
flags & VM_READ ? 'r' : '-',
@@ -99,7 +99,9 @@
flags & VM_EXEC ? 'x' : '-',
flags & VM_MAYSHARE ? 's' : 'p',
map->vm_pgoff << PAGE_SHIFT,
- MAJOR(dev), MINOR(dev), ino, &len);
+ MAJOR(dev), MINOR(dev), ino);
+ if (len < 0)
+ return 0;
if (map->vm_file) {
len = 25 + sizeof(void*) * 6 - len;
diff -Naurb -X /home/muli/w/dontdiff linux-2.5/fs/seq_file.c 2.6.5-rc3-pn/fs/seq_file.c
--- linux-2.5/fs/seq_file.c 2003-10-02 02:26:24.000000000 +0300
+++ 2.6.5-rc3-pn/fs/seq_file.c 2004-04-06 11:11:10.000000000 +0200
@@ -290,6 +290,16 @@
EXPORT_SYMBOL(seq_escape);
+/**
+ * seq_printf - Format a string and place it in a buffer
+ * @m: Target buffer
+ * @f: Printf format string
+ * @...: Arguments for the format string
+ *
+ * The return value is the number of characters which have been written into
+ * the buffer @m not including the trailing '\0'. If the buffer is too small,
+ * -1 is returned.
+ */
int seq_printf(struct seq_file *m, const char *f, ...)
{
va_list args;
@@ -297,11 +307,11 @@
if (m->count < m->size) {
va_start(args, f);
- len = vsnprintf(m->buf + m->count, m->size - m->count, f, args);
+ len = vscnprintf(m->buf + m->count, m->size - m->count, f, args);
va_end(args);
if (m->count + len < m->size) {
m->count += len;
- return 0;
+ return len;
}
}
m->count = m->size;
diff -Naurb -X /home/muli/w/dontdiff linux-2.5/lib/vsprintf.c 2.6.5-rc3-pn/lib/vsprintf.c
--- linux-2.5/lib/vsprintf.c 2004-04-06 11:34:50.000000000 +0200
+++ 2.6.5-rc3-pn/lib/vsprintf.c 2004-04-06 11:36:23.000000000 +0200
@@ -14,6 +14,9 @@
* - changed to provide snprintf and vsnprintf functions
* So Feb 1 16:51:32 CET 2004 Juergen Quade <[email protected]>
* - scnprintf and vscnprintf
+ * Sat Mar 20 22:38:09 2004 Muli Ben-Yehuda <[email protected]>
+ * - remove '%n' support from vsnprintf, as nothing is using it, and it
+ * has very few legitimate uses (and many many illegitimate ones)
*/
#include <stdarg.h>
@@ -401,22 +404,6 @@
16, field_width, precision, flags);
continue;
-
- case 'n':
- /* FIXME:
- * What does C99 say about the overflow case here? */
- if (qualifier == 'l') {
- long * ip = va_arg(args, long *);
- *ip = (str - buf);
- } else if (qualifier == 'Z' || qualifier == 'z') {
- size_t * ip = va_arg(args, size_t *);
- *ip = (str - buf);
- } else {
- int * ip = va_arg(args, int *);
- *ip = (str - buf);
- }
- continue;
-
case '%':
if (str <= end)
*str = '%';
--
Muli Ben-Yehuda
http://www.mulix.org | http://mulix.livejournal.com/
On Thu, Apr 08, 2004 at 12:18:15AM +0300, Muli Ben-Yehuda wrote:
> Hi,
>
> This patch removes support for the %n format specifier from
> vsprintf, remove the sole use of %n in the kernel, fixes seq_printf to
> return the number of characters written rather than 0 on success
> (thus getting rid of %n), and fixes the only two callers of seq_printf
> who bothered to look at the return value.
>
> The printf man page has this to say about '%n':
>
> "The number of characters written so far is stored into the integer
> indicated by the int * (or variant) pointer argument. No argument is
> converted."
>
> Very little code actually uses %n for that. These days, %n has a much
> more common use - in printf format string exploits. Since there's only
> one user of %n in the kernel, this patch removes support for it and
> fixes the user. To preempt the obvious argument, I agree that printk
> should look and behave as much as possible as printf - except where
> it's harmful. We don't support floating point, for example, and I
> doubt we should support %n - although I don't strongly care one way or
> another.
IMO that's wrong - if we get untrusted format we are screwed anyway (shove
enough %s and you are pretty much guaranteed to get an oops) and it's more
than likely that check for return value being negative will be forgotten.