linux-kernel - seq_file dangerous assumption?

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAFkjPTkmgMY_TkUHvCmN0iQ9Hx4ADBVCLXGrQB8uMyoF0xMj0g@mail.gmail.com>
Date:	Mon, 4 Jun 2012 14:32:02 -0500
From:	Eric Van Hensbergen <ericvh@...il.com>
To:	linux-kernel <linux-kernel@...r.kernel.org>
Subject: seq_file dangerous assumption?

I was merging up someone else's driver code from a much older kernel
to 3.5-rc1 and ran into some issues with corrupted memory.  The
character driver in question was using seq-file.c to handle reads to
the device.  Based on looking around at other drivers, no one else
does this -- so its probably (well, definitely based on what I found)
not the right way to do this.

seq_open seems to make a fairly general assumption:
(from linux-3.5-rc1 fs/seq_file.c)
...
int seq_open(struct file *file, const struct seq_operations *op)
{
        struct seq_file *p = file->private_data;

        if (!p) {
                p = kmalloc(sizeof(*p), GFP_KERNEL);
                if (!p)
                        return -ENOMEM;
                file->private_data = p;
        }
        memset(p, 0, sizeof(*p));
..

In other words, if something is in file->private_data, then we must
have already allocated and put our structure there.  In the case of
this driver, file->private_data was already populated (with a pointer
to the device structure) -- so the call to seq_open zero'd a portion
of the device structure and then corrupted it with a seq_file
structure.

So, an obvious solution is, don't use seq_file with a character device
-- but shouldn't there also be a fingerprint or something in the
seq_file structure as a sanity check so foolish developers don't trip
over it and corrupt their kernel memory?

      -eric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/