Re: [patch 03/26] sysfs: zero terminate sysfs write buffers (CVE-2006-1055)

[Al Viro <viro@ftp.linux.org.uk>]

On Wed, Apr 05, 2006 at 03:58:15PM -0400, Valdis.Kletnieks@vt.edu wrote:
> On Wed, 05 Apr 2006 16:39:57 BST, Al Viro said:
> 
> > How about _NOT_ using sysfs and just having ->read()/->write() on a file in fs
> > of your own?  ~20 lines for all of it, not counting #include...
> 
> Great.  Instead of everybody using the same piece-of-manure sysfs interface,
> each driver carries around its 20 lines to implement read() and write() in
> subtly buggy and incompatible ways.

No, that would be 20 lines to tell what and where you want in that fs and
how long should the things live.  Plus whatever you've got for your ->read()
and ->write() - using existing libfs helpers if needed.  Instead of pushing
into sysfs the things that do not fit sysfs interfaces.

BTW, in my experience "subtly buggy and incompatible ways" describes sysfs
uses, except that there's rarely anything subtle about that.  Care to name
four kernel data structures that got kobjects embedded into them (directly
or via struct device and it ilk) and had _NOT_ required at one point or
another (post-merge) fixing of blatant user-exploitable holes due to botched
lifetime rules?

Not that you had to embed them to achieve the same wonderful effect -
witness fbsysfs.c user-exploitable holes on unregister_framebuffer();
sure, fb_info->class_device will stay allocated if you have one of the
attributes opened.  Now try to call read(); what will it access?

Not to mention that the same file has a pile of ->store() assuming we
have NUL-termination, or the lovely use of sscanf() on non-NUL-terminated
array right in store_cmap() itself.  Equivalent of
	p = malloc(5);
	if (p) {
		memcpy(p, q, 5);
		sscanf(p, "%4hx", &v);
	}
You do realize that it's broken, don't you?  sscanf field width for %x
applies _after_ skipping the whitespace, not to the total amount of
characters being eaten.  And in reality this buffer comes from the end
of get_zeroed_page() result, so there's really nothing past its end.