Re: [PATCH] kmsg: Use vmalloc instead of kmalloc when writing

[Thomas Gleixner <tglx@linutronix.de>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2044 bytes --]

On Fri, 30 Mar 2012, Greg KH wrote:

> On Fri, Mar 30, 2012 at 10:35:46PM +0200, Thomas Gleixner wrote:
> > On Fri, 30 Mar 2012, Greg KH wrote:
> > > On Fri, Mar 30, 2012 at 07:37:37PM +0300, Sasha Levin wrote:
> > > > On Fri, Mar 30, 2012 at 6:30 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > On Fri, Mar 30, 2012 at 01:04:27PM -0400, Sasha Levin wrote:
> > > > >> There are no size checks in kmsg_write(), and we try allocating enough
> > > > >> memory to store everything userspace gave us, which may be too much for
> > > > >> kmalloc to allocate.
> > > > >
> > > > > Really?  Have you seen this fail?  As only root can do this, is this
> > > > > really a problem?
> > > > 
> > > > Only root, and a whole bunch of management software that dumps data
> > > > into /dev/kmsg (systemd and friends).
> > > 
> > > Running as root, do any of these cause problems by asking for too much
> > > memory here? 
> > 
> > Running as root is not a guarantee for correctness. So the syscall
> > should cope with bogus requests from user space and not rely on the
> > sanity of anything. Looking at the main users which polute dmesg I'm
> > inclined to assume insanity in the first place.
> > 
> > As Sasha pointed out there is either the variant to use vmalloc and
> > grant any write size or limit the size to something sensible. Though
> > given the users of this, coming up with something sensible might be a
> > problem.
> > 
> > > Is this something that needs to be addressed now, and in
> > > stable kernels, or can it wait for 3.5?
> > 
> > Yes, it want's to be addressed now and it want's to be in stable as
> > well. syscalls which have no bound checking are evil, no matter what.
> 
> So, should we cap the size at something "super large" then as well?

I think so. This is an interface to inject stuff into dmesg. Limiting
that to a reasonable size makes sense. We can probably limit it to
something small like 1024, but I don't know about the "ideas" of those
folks who think that it's a great idea to do it at all.

Thanks,

	tglx