seq_file dangerous assumption?

[Eric Van Hensbergen]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/