lkml.org 
[lkml]   [2018]   [Apr]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH?] reiserfs: prevent panic: don't allow %-char in journal dev. name
    From
    Date
    On 2018-04-05 03:45, Andrew Morton wrote:
    > On Wed, 4 Apr 2018 18:25:16 -0700 Randy Dunlap <rdunlap@infradead.org> wrote:
    >
    >> From: Randy Dunlap <rdunlap@infradead.org>
    >>
    >> If the reiserfs mount option's journal name contains a '%' character,
    >> it can lead to a WARN_ONCE() in lib/vsprintf.c::format_decode(),
    >> saying: "Please remove unsupported %/ in format string."
    >> That's OK until panic_on_warn is set, at which point it's dead, Jim.
    >>
    >> To placate this situation, check the journal name string for a '%'
    >> character and return an error if one is found. Also print a warning
    >> (one that won't panic the kernel) about the invalid journal name (e.g.):
    >>
    >> reiserfs: journal device name is invalid: %/file0
    >>
    >> (In this example, the caller app specified the journal device name as
    >> "%/file0".)
    >>
    >
    > Well, that is a valid filename and we should support it...
    >
    > Isn't the bug in journal_init_dev()?

    Urgh. At first I was about to reply that the real bug was in reiserfs.h
    for failing to annotate __reiserfs_warning with __printf(). But digging
    into it, it turns out that it implements its own printf extensions, so
    that's obviously a non-starter. Now, one thing is that some of those
    extension clash with existing standard modifiers (%z and %h, so if
    someone adds a correct %zu thing to print a size_t in reiserfs things
    will break). But, and I hope I'm wrong about this and just hasn't had
    enough coffee, this seems completely broken:

    while ((k = is_there_reiserfs_struct(fmt1, &what)) != NULL) {
    *k = 0;

    p += vsprintf(p, fmt1, args);

    switch (what) {
    case 'k':
    sprintf_le_key(p, va_arg(args, struct
    reiserfs_key *));
    break;

    On architectures where va_list is a typedef for a one-element array of
    some struct (x86-64), that works ok, because the vsprintf call can and
    does update the args metadata. But when args is just a pointer into the
    stack (i386), we don't know how much vsprintf consumed, and end up
    consuming the same arguments again - only this time we may interpret
    some random integer as a struct pointer...

    A minimal program showing the difference:

    #include <stdio.h>
    #include <stdarg.h>

    void f(const char *dummy, ...)
    {
    va_list ap;
    int i;

    va_start(ap, dummy);
    for (i = 0; i < 5; ++i) {
    vprintf("%d\n", ap);
    printf("%d\n", va_arg(ap, int));
    }
    va_end(ap);
    }

    int main(int argc, char *argv[])
    {
    f("bla", 1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
    return 0;
    }

    Compiling for native (x86-64), this produces $(seq 10). But with -m32,
    one gets 1,1,2,2,3,3,4,4,5,5.

    Assuming reiserfs (at least its debugging infrastructure) isn't broken
    on a bunch of architectures, I'm obviously missing something
    fundamental. Please enlighten me.

    Rasmus

    \
     
     \ /
      Last update: 2018-04-05 11:05    [W:3.233 / U:0.080 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site