In-Reply-To: <[email protected]>
On Sun, 5 Mar 2006 19:27:53 -0800, Linus Torvalds wrote:
> So I'd be more inclined to blame a buffer overflow on a kmalloc, and the
> obvious target is the "add_uevent_var()" thing, since all/many of the
> corruptions seem to come from uevent environment variable strings.
At least one susbsystem rolls its own method of adding env vars to the
uevent buffer, and it's so broken it triggers the WARN_ON() in
lib/vsprintf.c::vsnprintf() by passing a negative length to that function.
Start at drivers/input/input.c::input_dev_uevent() and watch the fun.
I reported this to linux-kernel, the input maintainer and the author
of that code on Feb. 26:
http://lkml.org/lkml/2006/2/26/39
--
Chuck
"Penguins don't come from next door, they come from the Antarctic!"
On Tue, Mar 07, 2006 at 04:54:24PM -0500, Chuck Ebbert wrote:
> In-Reply-To: <[email protected]>
>
> On Sun, 5 Mar 2006 19:27:53 -0800, Linus Torvalds wrote:
>
> > So I'd be more inclined to blame a buffer overflow on a kmalloc, and the
> > obvious target is the "add_uevent_var()" thing, since all/many of the
> > corruptions seem to come from uevent environment variable strings.
>
> At least one susbsystem rolls its own method of adding env vars to the
> uevent buffer, and it's so broken it triggers the WARN_ON() in
> lib/vsprintf.c::vsnprintf() by passing a negative length to that function.
> Start at drivers/input/input.c::input_dev_uevent() and watch the fun.
All of the INPUT_ADD_HOTPLUG_VAR() calls do use add_uevent_var(), so we
should be safe there. The other calls also look safe, if not a bit
wierd... So I don't see how we could change this to be any safer, do
you?
> I reported this to linux-kernel, the input maintainer and the author
> of that code on Feb. 26:
>
> http://lkml.org/lkml/2006/2/26/39
We should have fixed that already by increasing the size of the buffer,
but yes, we should catch errors in the MODALIAS function, that would
have stopped that previous overflow. Are you still seeing problems now?
thanks,
greg k-h
On Tue, 7 Mar 2006, Chuck Ebbert wrote:
>
> At least one susbsystem rolls its own method of adding env vars to the
> uevent buffer, and it's so broken it triggers the WARN_ON() in
> lib/vsprintf.c::vsnprintf() by passing a negative length to that function.
Well, snprintf() should be safe, though. It will warn if the caller is
lazy, but these days, the thing does
max(buf_size - len, 0)
which should mean that the input layer passes in 0 instead of a negative
number. And snprintf() will then _not_ print anything.
So I think input_add_uevent_bm_var() is safe, even if it's not pretty.
However, input_devices_read() doesn't do any sanity checking at all, and
if that ever ends up printing more than a page, that would be bad. I
didn't look very closely, but it looks worrisome.
Dmitry?
Linus
On Tuesday 07 March 2006 19:57, Linus Torvalds wrote:
>
> On Tue, 7 Mar 2006, Chuck Ebbert wrote:
> >
> > At least one susbsystem rolls its own method of adding env vars to the
> > uevent buffer, and it's so broken it triggers the WARN_ON() in
> > lib/vsprintf.c::vsnprintf() by passing a negative length to that function.
>
> Well, snprintf() should be safe, though. It will warn if the caller is
> lazy, but these days, the thing does
>
> max(buf_size - len, 0)
>
> which should mean that the input layer passes in 0 instead of a negative
> number. And snprintf() will then _not_ print anything.
>
> So I think input_add_uevent_bm_var() is safe, even if it's not pretty.
>
> However, input_devices_read() doesn't do any sanity checking at all, and
> if that ever ends up printing more than a page, that would be bad. I
> didn't look very closely, but it looks worrisome.
>
> Dmitry?
I had all this code converted to seq_file, but it depends on converting
input handlers to class interfaces and it is not possible nowadays
because with latest Greg's changes to class code we would try to
register class devices while registering class devices/interfaces
(psmouse creates input_dev which binds to mousedev interface which in
turn tries to create mouseX which is also belongs to input class) and
deadlocking. Greg promised current implementation is only a temporary
solution.
I suppose I could separate those changes...
BTW, what is ETA for 2.6.16 - I could not affort enought time getting
psmouse resync logic working in all cases so I'd like to have it disabled
for 2.6.16 final.
--
Dmitry
On Tue, Mar 07, 2006 at 08:13:28PM -0500, Dmitry Torokhov wrote:
> On Tuesday 07 March 2006 19:57, Linus Torvalds wrote:
> >
> > On Tue, 7 Mar 2006, Chuck Ebbert wrote:
> > >
> > > At least one susbsystem rolls its own method of adding env vars to the
> > > uevent buffer, and it's so broken it triggers the WARN_ON() in
> > > lib/vsprintf.c::vsnprintf() by passing a negative length to that function.
> >
> > Well, snprintf() should be safe, though. It will warn if the caller is
> > lazy, but these days, the thing does
> >
> > max(buf_size - len, 0)
> >
> > which should mean that the input layer passes in 0 instead of a negative
> > number. And snprintf() will then _not_ print anything.
> >
> > So I think input_add_uevent_bm_var() is safe, even if it's not pretty.
> >
> > However, input_devices_read() doesn't do any sanity checking at all, and
> > if that ever ends up printing more than a page, that would be bad. I
> > didn't look very closely, but it looks worrisome.
> >
> > Dmitry?
>
> I had all this code converted to seq_file, but it depends on converting
> input handlers to class interfaces and it is not possible nowadays
> because with latest Greg's changes to class code we would try to
> register class devices while registering class devices/interfaces
> (psmouse creates input_dev which binds to mousedev interface which in
> turn tries to create mouseX which is also belongs to input class) and
> deadlocking. Greg promised current implementation is only a temporary
> solution.
>
> I suppose I could separate those changes...
That would probably be a good idea :)
Oh, and I have been working on makeing the correct fix, you can see the
initial try of it at:
http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/device-class.patch
if you are really brave :)
thanks,
greg k-h
On Tuesday 07 March 2006 20:27, Greg KH wrote:
> On Tue, Mar 07, 2006 at 08:13:28PM -0500, Dmitry Torokhov wrote:
> > On Tuesday 07 March 2006 19:57, Linus Torvalds wrote:
> > >
> > > On Tue, 7 Mar 2006, Chuck Ebbert wrote:
> > > >
> > > > At least one susbsystem rolls its own method of adding env vars to the
> > > > uevent buffer, and it's so broken it triggers the WARN_ON() in
> > > > lib/vsprintf.c::vsnprintf() by passing a negative length to that function.
> > >
> > > Well, snprintf() should be safe, though. It will warn if the caller is
> > > lazy, but these days, the thing does
> > >
> > > max(buf_size - len, 0)
> > >
> > > which should mean that the input layer passes in 0 instead of a negative
> > > number. And snprintf() will then _not_ print anything.
> > >
> > > So I think input_add_uevent_bm_var() is safe, even if it's not pretty.
> > >
> > > However, input_devices_read() doesn't do any sanity checking at all, and
> > > if that ever ends up printing more than a page, that would be bad. I
> > > didn't look very closely, but it looks worrisome.
> > >
> > > Dmitry?
> >
> > I had all this code converted to seq_file, but it depends on converting
> > input handlers to class interfaces and it is not possible nowadays
> > because with latest Greg's changes to class code we would try to
> > register class devices while registering class devices/interfaces
> > (psmouse creates input_dev which binds to mousedev interface which in
> > turn tries to create mouseX which is also belongs to input class) and
> > deadlocking. Greg promised current implementation is only a temporary
> > solution.
> >
> > I suppose I could separate those changes...
>
> That would probably be a good idea :)
>
Hmm, what is the policy for attr->show()? With hotplug variables we
return -ENOMEM if there is not enough memory to store all data, but
what about attributes? Should we also return error (and which one,
-ENOMEM, -ENOBUFS?) or fill as much as we can and return up to
PAGE_SIZE? With sysfs not kernel nor application can really recover
if attribute needs buffer larger than a page. Or just rely on BUG_ON
in fs/sysfs/file.c::fill_read_buffer()?
--
Dmitry
On Tue, Mar 07, 2006 at 04:57:39PM -0800, Linus Torvalds wrote:
> Well, snprintf() should be safe, though. It will warn if the caller is
> lazy, but these days, the thing does
>
> max(buf_size - len, 0)
>
> which should mean that the input layer passes in 0 instead of a negative
> number. And snprintf() will then _not_ print anything.
I assume this is a typo, and you meant scnprintf? AFAIK, snprintf has
the same ol' bad behavior when #bytes-to-be-written > #bytes-in-buffer.
Joe
On Tuesday 07 March 2006 23:28, Joe Korty wrote:
> On Tue, Mar 07, 2006 at 04:57:39PM -0800, Linus Torvalds wrote:
>
> > Well, snprintf() should be safe, though. It will warn if the caller is
> > lazy, but these days, the thing does
> >
> > max(buf_size - len, 0)
> >
> > which should mean that the input layer passes in 0 instead of a negative
> > number. And snprintf() will then _not_ print anything.
>
> I assume this is a typo, and you meant scnprintf? AFAIK, snprintf has
> the same ol' bad behavior when #bytes-to-be-written > #bytes-in-buffer.
>
No, we do want to know if output was truncated or not so snprintf is used.
--
Dmitry
On Tue, Mar 07, 2006 at 10:22:10PM -0500, Dmitry Torokhov wrote:
> On Tuesday 07 March 2006 20:27, Greg KH wrote:
> > On Tue, Mar 07, 2006 at 08:13:28PM -0500, Dmitry Torokhov wrote:
> > > On Tuesday 07 March 2006 19:57, Linus Torvalds wrote:
> > > >
> > > > On Tue, 7 Mar 2006, Chuck Ebbert wrote:
> > > > >
> > > > > At least one susbsystem rolls its own method of adding env vars to the
> > > > > uevent buffer, and it's so broken it triggers the WARN_ON() in
> > > > > lib/vsprintf.c::vsnprintf() by passing a negative length to that function.
> > > >
> > > > Well, snprintf() should be safe, though. It will warn if the caller is
> > > > lazy, but these days, the thing does
> > > >
> > > > max(buf_size - len, 0)
> > > >
> > > > which should mean that the input layer passes in 0 instead of a negative
> > > > number. And snprintf() will then _not_ print anything.
> > > >
> > > > So I think input_add_uevent_bm_var() is safe, even if it's not pretty.
> > > >
> > > > However, input_devices_read() doesn't do any sanity checking at all, and
> > > > if that ever ends up printing more than a page, that would be bad. I
> > > > didn't look very closely, but it looks worrisome.
> > > >
> > > > Dmitry?
> > >
> > > I had all this code converted to seq_file, but it depends on converting
> > > input handlers to class interfaces and it is not possible nowadays
> > > because with latest Greg's changes to class code we would try to
> > > register class devices while registering class devices/interfaces
> > > (psmouse creates input_dev which binds to mousedev interface which in
> > > turn tries to create mouseX which is also belongs to input class) and
> > > deadlocking. Greg promised current implementation is only a temporary
> > > solution.
> > >
> > > I suppose I could separate those changes...
> >
> > That would probably be a good idea :)
> >
>
> Hmm, what is the policy for attr->show()? With hotplug variables we
> return -ENOMEM if there is not enough memory to store all data, but
> what about attributes? Should we also return error (and which one,
> -ENOMEM, -ENOBUFS?) or fill as much as we can and return up to
> PAGE_SIZE?
Remember, sysfs files are supposed to be small, you are an "oddity" in
that you have a much larger buffer that you can return due to the wierd
aliases you have.
Truncating the buffer is probably good as we want userspace to get some
information, right?
> With sysfs not kernel nor application can really recover
> if attribute needs buffer larger than a page. Or just rely on BUG_ON
> in fs/sysfs/file.c::fill_read_buffer()?
How about just making this a binary attribute, then you can handle an
arbitrary size buffer and don't have to worry about the PAGE_SIZE stuff
(but it makes it more code that you have to write to handle it all,
there are tradeoffs...)
thanks,
greg k-h
On Wednesday 08 March 2006 00:23, Greg KH wrote:
> >
> > Hmm, what is the policy for attr->show()? With hotplug variables we
> > return -ENOMEM if there is not enough memory to store all data, but
> > what about attributes? Should we also return error (and which one,
> > -ENOMEM, -ENOBUFS?) or fill as much as we can and return up to
> > PAGE_SIZE?
>
> Remember, sysfs files are supposed to be small, you are an "oddity" in
> that you have a much larger buffer that you can return due to the wierd
> aliases you have.
>
> Truncating the buffer is probably good as we want userspace to get some
> information, right?
>
> > With sysfs not kernel nor application can really recover
> > if attribute needs buffer larger than a page. Or just rely on BUG_ON
> > in fs/sysfs/file.c::fill_read_buffer()?
>
> How about just making this a binary attribute, then you can handle an
> arbitrary size buffer and don't have to worry about the PAGE_SIZE stuff
> (but it makes it more code that you have to write to handle it all,
> there are tradeoffs...)
>
I really don't believe that we ever going to cross 4096 boundary for any
single input attribute, but just to code defensively we need to decide
what to do if we ever encounter a crazy device. Just truncating to
PAGE_SIZE is easiest so that's what I am going to do.
--
Dmitry
On Wed, Mar 08, 2006 at 01:06:59AM -0500, Dmitry Torokhov wrote:
> On Wednesday 08 March 2006 00:23, Greg KH wrote:
> > >
> > > Hmm, what is the policy for attr->show()? With hotplug variables we
> > > return -ENOMEM if there is not enough memory to store all data, but
> > > what about attributes? Should we also return error (and which one,
> > > -ENOMEM, -ENOBUFS?) or fill as much as we can and return up to
> > > PAGE_SIZE?
> >
> > Remember, sysfs files are supposed to be small, you are an "oddity" in
> > that you have a much larger buffer that you can return due to the wierd
> > aliases you have.
> >
> > Truncating the buffer is probably good as we want userspace to get some
> > information, right?
> >
> > > With sysfs not kernel nor application can really recover
> > > if attribute needs buffer larger than a page. Or just rely on BUG_ON
> > > in fs/sysfs/file.c::fill_read_buffer()?
> >
> > How about just making this a binary attribute, then you can handle an
> > arbitrary size buffer and don't have to worry about the PAGE_SIZE stuff
> > (but it makes it more code that you have to write to handle it all,
> > there are tradeoffs...)
> >
>
> I really don't believe that we ever going to cross 4096 boundary for any
> single input attribute, but just to code defensively we need to decide
> what to do if we ever encounter a crazy device. Just truncating to
> PAGE_SIZE is easiest so that's what I am going to do.
Ok, that's fine with me.
thanks,
greg k-h