In-Reply-To: <[email protected]>
On Tue, 7 Mar 2006 16:54:55 -0800, Greg KH wrote:
> On Tue, Mar 07, 2006 at 04:54:24PM -0500, 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.
> > 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?
input.c line 747+ was recently added and caused the error message:
[1]=> envp[i++] = buffer + len;
[2]=> len += snprintf(buffer + len, buffer_size - len, "MODALIAS=");
[2]=> len += print_modalias(buffer + len, buffer_size - len, dev) + 1;
[1]=> envp[i] = NULL;
return 0;
[1] What is checking for enough space here? This didn't overflow AFAICT
but it could.
[2] The snprintf() and print_modalias() calls don't check for errors and
thus don't return -ENOMEM when the buffer does fill up. Shouldn't they
do that instead of returning a truncated env string? Only the final
sanity test in snprintf() keeps them from overrunning the buffer.
(It was print_modalias_bits() that actually caused the overflow.)
> Are you still seeing problems now?
Wasn't me, it was Horst Schirmeier <[email protected]>
--
Chuck
"Penguins don't come from next door, they come from the Antarctic!"
On Tuesday 07 March 2006 22:29, Chuck Ebbert wrote:
> In-Reply-To: <[email protected]>
>
> On Tue, 7 Mar 2006 16:54:55 -0800, Greg KH wrote:
>
> > On Tue, Mar 07, 2006 at 04:54:24PM -0500, 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.
> > > 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?
>
> input.c line 747+ was recently added and caused the error message:
>
> [1]=> envp[i++] = buffer + len;
> [2]=> len += snprintf(buffer + len, buffer_size - len, "MODALIAS=");
> [2]=> len += print_modalias(buffer + len, buffer_size - len, dev) + 1;
>
> [1]=> envp[i] = NULL;
> return 0;
>
> [1] What is checking for enough space here? This didn't overflow AFAICT
> but it could.
>
> [2] The snprintf() and print_modalias() calls don't check for errors and
> thus don't return -ENOMEM when the buffer does fill up. Shouldn't they
> do that instead of returning a truncated env string? Only the final
> sanity test in snprintf() keeps them from overrunning the buffer.
> (It was print_modalias_bits() that actually caused the overflow.)
>
I agree with all of the above, it will be fixed.
--
Dmitry
On Tue, 7 Mar 2006, Chuck Ebbert wrote:
>
> [2] The snprintf() and print_modalias() calls don't check for errors and
> thus don't return -ENOMEM when the buffer does fill up. Shouldn't they
> do that instead of returning a truncated env string?
They try to act like the standard says "snprintf()" should act.
And yes, the standard says to return the number of bytes you _would_ have
written, had not the buffer been to small.
(Of course, giving a negative buffer length is not ok, and the kernel
version checking for that is a kernel extension on the standard. In the
standard, the buffer size is a "size_t", which doesn't have the notion of
"negative", since it's an unsigned type. The kernel version is just being
safe and nice).
Linus