Messages in this thread | | | Date | Mon, 4 Jun 2012 22:07:20 +0200 | From | Jan Kara <> | Subject | Re: seq_file dangerous assumption? |
| |
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
| |