Re: seq_file dangerous assumption?

[Jan Kara <jack@suse.cz>]

On Mon 04-06-12 14:32:02, Eric Van Hensbergen wrote:
> 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?
  Well, seq_file was never though to be used for devices... It was written
for use by virtual files such as those in /proc. Thus noone really thought
of problems you hit.

Also we don't usually put magics into our data structure just to stop bad
use of interfaces. I agree that in this particular case the interface is
easy to get wrong - but that should be solved by changing the interface to
a more robust one. Actually, I'm not sure if anyone actually passes
->private_data != NULL since seq_open_private() seems to be a standard way
of associating some additional data with seq_file. So maybe
BUG_ON(file->private_data) would be a good robustification of the interface
:).

								Honza

-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR