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

[Jon Smirl]

On 4/5/06, Sergey Vlasov <vsu@xxxxxxxxxxx> wrote:
> On Tue, 4 Apr 2006 16:59:47 -0700 gregkh@xxxxxxx wrote:
>
> > No one should be writing a PAGE_SIZE worth of data to a normal sysfs
> > file, so properly terminate the buffer.
> >
> > Thanks to Al Viro for pointing out my stupidity here.
> >
> > CVE-2006-1055 has been assigned for this.
> >
> > Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxx>
> >
> > ---
> >  fs/sysfs/file.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > --- linux-2.6.16.1.orig/fs/sysfs/file.c
> > +++ linux-2.6.16.1/fs/sysfs/file.c
> > @@ -183,7 +183,7 @@ fill_write_buffer(struct sysfs_buffer *
> >               return -ENOMEM;
> >
> >       if (count >= PAGE_SIZE)
> > -             count = PAGE_SIZE;
> > +             count = PAGE_SIZE - 1;
> >       error = copy_from_user(buffer->page,buf,count);
> >       buffer->needs_read_fill = 1;
> >       return error ? -EFAULT : count;
>
> This will break the "color_map" sysfs file for framebuffers -
> drivers/video/fbsysfs.c:store_cmap() expects to get exactly 4096 bytes
> for a colormap with 256 entries.  In fact, the original patch which
> changed PAGE_SIZE - 1 to PAGE_SIZE:
>
> http://kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=9d9d27fb651a7c95a46f276bacb4329db47470a6
>
> was done exactly for use with that "color_map" file.
>
> This patch also does not completely guarantee that the buffer will be
> null-terminated.  A program may first call read() on the sysfs file,
> which will allocate buffer->page and invoke ->show to fill that page;
> then subsequent write() on the same file will reuse buffer->page.  To
> get really bad results, you need to have ->store which assumes
> null-terminated buffer together with ->show which writes to the last
> byte of the page (which is probably rare, but show_cmap() does exactly
> that).
>

That is correct, that the color_map attribute will break. Color_map is
not in general use outside the Mesa development community.

The whole scheme of using sysfs instead of IOCTLs is not working out
very well for framebuffer. The original idea was to let you control
your framebuffer with simple scripts or from the keyboard. But since
the attributes don't strip \n and blanks, it has made them more
complex to use from the keyboard.

The one attribute per file model doesn't work well when the attributes
need to be changed in a transaction. For example you want to change
your display to 1024x768 16bit color.  As you set the attributes one
at a time the display has to change since there is not guarantee that
you will complete the sequence. The framebuffer sysfs interface breaks
the one attribute per file rule and uses strings for grouped
attributes.

Ultimately I expect framebuffer will switch back to a helper app and
binary IOCTLs. Mainly because the help app can signal begin/end around
a change to a group of attributes.

--
Jon Smirl
jonsmirl@xxxxxxxxx
-
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/