[PATCH 1/2] uevent: don't pass envp_ext[] as format string in kobject_uevent_env()

[PATCH 1/2] uevent: don't pass envp_ext[] as format string in kobject_uevent_env()

[Tejun Heo]

kobject_uevent_env() uses envp_ext[] as verbatim format string which
can cause problems ranging from unexpectedly mangled string to oops if
a string in envp_ext[] contains substring which can be interpreted as
format.  Fix it.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 lib/kobject_uevent.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
index 3f91472..ca215bc 100644
--- a/lib/kobject_uevent.c
+++ b/lib/kobject_uevent.c
@@ -165,7 +165,7 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
 	/* keys passed in from the caller */
 	if (envp_ext) {
 		for (i = 0; envp_ext[i]; i++) {
-			retval = add_uevent_var(env, envp_ext[i]);
+			retval = add_uevent_var(env, "%s", envp_ext[i]);
 			if (retval)
 				goto exit;
 		}
-- 
1.5.4.5

[PATCH 2/2] uevent: handle duplicate uevent_var keys properly

[Tejun Heo]

add_uevent_var() appends the specified variable whether the new entry
has duplicate key or not.  This patch makes add_uevent_var() to
override the existing entry if an entry with the same key is added
later.  This will be used by CUSE (character device in userland) to
fake hotplug events.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 lib/kobject_uevent.c |   33 ++++++++++++++++++++++-----------
 1 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
index ca215bc..64b1aaf 100644
--- a/lib/kobject_uevent.c
+++ b/lib/kobject_uevent.c
@@ -281,27 +281,38 @@ EXPORT_SYMBOL_GPL(kobject_uevent);
  */
 int add_uevent_var(struct kobj_uevent_env *env, const char *format, ...)
 {
+	char *buf = &env->buf[env->buflen];
+	size_t avail = sizeof(env->buf) - env->buflen;
 	va_list args;
-	int len;
+	int i, len, key_len;
 
-	if (env->envp_idx >= ARRAY_SIZE(env->envp)) {
-		WARN(1, KERN_ERR "add_uevent_var: too many keys\n");
+	va_start(args, format);
+	len = vsnprintf(buf, avail, format, args);
+	va_end(args);
+
+	if (len >= avail) {
+		printk(KERN_ERR "add_uevent_var: buffer size too small\n");
+		WARN_ON(1);
 		return -ENOMEM;
 	}
 
-	va_start(args, format);
-	len = vsnprintf(&env->buf[env->buflen],
-			sizeof(env->buf) - env->buflen,
-			format, args);
-	va_end(args);
+	key_len = strchr(buf, '=') - buf;
+
+	for (i = 0; i < env->envp_idx; i++)
+		if (key_len == strchr(env->envp[i], '=') - env->envp[i] &&
+		    !strncmp(buf, env->envp[i], key_len))
+			break;
 
-	if (len >= (sizeof(env->buf) - env->buflen)) {
-		WARN(1, KERN_ERR "add_uevent_var: buffer size too small\n");
+	if (i >= ARRAY_SIZE(env->envp)) {
+		printk(KERN_ERR "add_uevent_var: too many keys\n");
+		WARN_ON(1);
 		return -ENOMEM;
 	}
 
-	env->envp[env->envp_idx++] = &env->buf[env->buflen];
+	env->envp[i] = &env->buf[env->buflen];
 	env->buflen += len + 1;
+	if (i == env->envp_idx)
+		env->envp_idx++;
 	return 0;
 }
 EXPORT_SYMBOL_GPL(add_uevent_var);
-- 
1.5.4.5

Re: [PATCH 2/2] uevent: handle duplicate uevent_var keys properly

[Greg KH]

On Thu, Aug 28, 2008 at 06:31:02PM +0200, Tejun Heo wrote:
> add_uevent_var() appends the specified variable whether the new entry
> has duplicate key or not.  This patch makes add_uevent_var() to
> override the existing entry if an entry with the same key is added
> later.  This will be used by CUSE (character device in userland) to
> fake hotplug events.

Hm, do you have any pointers to CUSE, that sounds interesting.

And how would this change interact with fake hotplug events?

thanks,

greg k-h

Re: [PATCH 2/2] uevent: handle duplicate uevent_var keys properly

[Tejun Heo]

Greg KH wrote:
> On Thu, Aug 28, 2008 at 06:31:02PM +0200, Tejun Heo wrote:
>> add_uevent_var() appends the specified variable whether the new entry
>> has duplicate key or not.  This patch makes add_uevent_var() to
>> override the existing entry if an entry with the same key is added
>> later.  This will be used by CUSE (character device in userland) to
>> fake hotplug events.
> 
> Hm, do you have any pointers to CUSE, that sounds interesting.

I'm in the process of sending patches.  I'll cc you on the actual postings.

> And how would this change interact with fake hotplug events?

CUSE creates actual devices but those devices are all cuse class
devices.  To play nicely with sysfs/hal, the ADD/REMOVE uevents should
have about the same variables as the actual device including the
SUBSYSTEM, so that's where the overriding comes in.  CUSE client tells
CUSE that it needs to set such such envs for uevents and CUSE overrides
uevents before sending it out so that sysfs/hal can be fooled.

Thanks.

-- 
tejun

Re: [PATCH 2/2] uevent: handle duplicate uevent_var keys properly

[Greg KH]

On Thu, Aug 28, 2008 at 07:00:39PM +0200, Tejun Heo wrote:
> Greg KH wrote:
> > On Thu, Aug 28, 2008 at 06:31:02PM +0200, Tejun Heo wrote:
> >> add_uevent_var() appends the specified variable whether the new entry
> >> has duplicate key or not.  This patch makes add_uevent_var() to
> >> override the existing entry if an entry with the same key is added
> >> later.  This will be used by CUSE (character device in userland) to
> >> fake hotplug events.
> > 
> > Hm, do you have any pointers to CUSE, that sounds interesting.
> 
> I'm in the process of sending patches.  I'll cc you on the actual postings.

Thanks, I'd appreciate that.

> > And how would this change interact with fake hotplug events?
> 
> CUSE creates actual devices but those devices are all cuse class
> devices.  To play nicely with sysfs/hal, the ADD/REMOVE uevents should
> have about the same variables as the actual device including the
> SUBSYSTEM, so that's where the overriding comes in.  CUSE client tells
> CUSE that it needs to set such such envs for uevents and CUSE overrides
> uevents before sending it out so that sysfs/hal can be fooled.

Heh, nice, sounds interesting.  I'll queue this patch up.

thanks,

greg k-h

Re: [PATCH 2/2] uevent: handle duplicate uevent_var keys properly

[Kay Sievers]

On Thu, Aug 28, 2008 at 19:00, Tejun Heo <tj@kernel.org> wrote:
> Greg KH wrote:
>> On Thu, Aug 28, 2008 at 06:31:02PM +0200, Tejun Heo wrote:
>>> add_uevent_var() appends the specified variable whether the new entry
>>> has duplicate key or not.  This patch makes add_uevent_var() to
>>> override the existing entry if an entry with the same key is added
>>> later.  This will be used by CUSE (character device in userland) to
>>> fake hotplug events.
>>
>> Hm, do you have any pointers to CUSE, that sounds interesting.
>
> I'm in the process of sending patches.  I'll cc you on the actual postings.
>
>> And how would this change interact with fake hotplug events?
>
> CUSE creates actual devices but those devices are all cuse class
> devices.  To play nicely with sysfs/hal, the ADD/REMOVE uevents should
> have about the same variables as the actual device including the
> SUBSYSTEM, so that's where the overriding comes in.  CUSE client tells
> CUSE that it needs to set such such envs for uevents and CUSE overrides
> uevents before sending it out so that sysfs/hal can be fooled.

Not sure if I understand that correctly. Remember, that there is a
symlink "subsystem" at each device, and udev, HAL, DeviceKit reads it.
If the uevent environment key "SUBSYSTEM" does not match the symlink
target, things will break horribly. So no device can be a member of
class "cuse" but carry a SUBSYSTEM value of a different class.

If that is how it works, I guess that must be solved differently, by
hooking into the subsystem code and create "virtual devices" at the
original class they fake, instead of their own "cuse" class. Possibly
by making cuse a "bus", and use the cuse device as a parent for the
"real" class device.

Thanks,
Kay

Re: [PATCH 2/2] uevent: handle duplicate uevent_var keys properly

[Tejun Heo]

Kay Sievers wrote:
> Not sure if I understand that correctly. Remember, that there is a
> symlink "subsystem" at each device, and udev, HAL, DeviceKit reads it.
> If the uevent environment key "SUBSYSTEM" does not match the symlink
> target, things will break horribly. So no device can be a member of
> class "cuse" but carry a SUBSYSTEM value of a different class.
>
> If that is how it works, I guess that must be solved differently, by
> hooking into the subsystem code and create "virtual devices" at the
> original class they fake, instead of their own "cuse" class. Possibly
> by making cuse a "bus", and use the cuse device as a parent for the
> "real" class device.

For OSS emulation, it didn't really matter.  For cases where it matters,
I think easier path to take would be let the userland emulation set up a
directory containing pseudo files and just override DEVPATH to it.
Would that work?

Thanks.

-- 
tejun

Re: [PATCH 2/2] uevent: handle duplicate uevent_var keys properly

[Kay Sievers]

On Fri, 2008-08-29 at 10:02 +0200, Tejun Heo wrote:
> Kay Sievers wrote:
> > Not sure if I understand that correctly. Remember, that there is a
> > symlink "subsystem" at each device, and udev, HAL, DeviceKit reads it.
> > If the uevent environment key "SUBSYSTEM" does not match the symlink
> > target, things will break horribly. So no device can be a member of
> > class "cuse" but carry a SUBSYSTEM value of a different class.
> >
> > If that is how it works, I guess that must be solved differently, by
> > hooking into the subsystem code and create "virtual devices" at the
> > original class they fake, instead of their own "cuse" class. Possibly
> > by making cuse a "bus", and use the cuse device as a parent for the
> > "real" class device.
> 
> For OSS emulation, it didn't really matter.  For cases where it matters,
> I think easier path to take would be let the userland emulation set up a
> directory containing pseudo files and just override DEVPATH to it.
> Would that work?

No, I don't think so, it's a too bad hack. the SUBSYSTEM key, the entry
in the class/bus directory, and the target of the symlink in the device
directory _must_ match. Everything else asks for serious trouble, by the
inconsistency it creates.

Thanks,
Kay

Re: [PATCH 2/2] uevent: handle duplicate uevent_var keys properly

[Tejun Heo]

Hello,

Kay Sievers wrote:
>> For OSS emulation, it didn't really matter.  For cases where it matters,
>> I think easier path to take would be let the userland emulation set up a
>> directory containing pseudo files and just override DEVPATH to it.
>> Would that work?
> 
> No, I don't think so, it's a too bad hack. the SUBSYSTEM key, the entry
> in the class/bus directory, and the target of the symlink in the device
> directory _must_ match. Everything else asks for serious trouble, by the
> inconsistency it creates.

I'm afraid trying to do that from kernel side would require more scary
hack as CUSE will need to push in random device under unsuspecting
parent / class.  Maybe we can add a variable to indicate an emulated
device or to tell udev/hal whatever to use alt root for sys hierarchy or
just consider sysfs is not available?  I think this problem can be
settled between userland programs.

Thanks.

-- 
tejun

Re: [PATCH 2/2] uevent: handle duplicate uevent_var keys properly

[Kay Sievers]

On Fri, 2008-08-29 at 15:34 +0200, Tejun Heo wrote:
> Kay Sievers wrote:
> >> For OSS emulation, it didn't really matter.  For cases where it matters,
> >> I think easier path to take would be let the userland emulation set up a
> >> directory containing pseudo files and just override DEVPATH to it.
> >> Would that work?
> > 
> > No, I don't think so, it's a too bad hack. the SUBSYSTEM key, the entry
> > in the class/bus directory, and the target of the symlink in the device
> > directory _must_ match. Everything else asks for serious trouble, by the
> > inconsistency it creates.
> 
> I'm afraid trying to do that from kernel side would require more scary
> hack as CUSE will need to push in random device under unsuspecting
> parent / class.  Maybe we can add a variable to indicate an emulated
> device or to tell

Sure, SUSBYSTEM=cuse will be exactly that value. :) I see no problem, if
cuse adds SUBSYSTEM_FAKE, or whatever it would be.

> udev/hal whatever to use alt root for sys hierarchy or
> just consider sysfs is not available?  I think this problem can be
> settled between userland programs.

Exactly, we can do anything needed in userspace to support that. I see
no problem doing that.

I just say, that we can _not_ allow to fake the SUBSYSTEM value for
"cuse" or any other device. If the kernel wants to let "cuse" devices
look like members of a specific subsystem, they have to appear in the
class/bus list of that subsystem, and have to have the proper
"susbsystem" link, not only a (wrong) environment key.

Userland enumerates devices by looking at class/bus directories for
things like: "give me all soundcards", it expects a set of certain sysfs
attributes to be present for a specific device, it reads the "subsystem"
link, and so on. All that would break and make things "special"  and
needlessly inconsistent.

So please drop that plan, of faking only a tiny part of
driver-core/sysfs device parameters. Either we do it properly, or we
don't do it, and leave it up to userspace to pick up a (consistent)
"cuse" device, but use it as a device of whatever class.

Thanks,
Kay

Re: [PATCH 2/2] uevent: handle duplicate uevent_var keys properly

[Tejun Heo]

Kay Sievers wrote:
>> I'm afraid trying to do that from kernel side would require more scary
>> hack as CUSE will need to push in random device under unsuspecting
>> parent / class.  Maybe we can add a variable to indicate an emulated
>> device or to tell
> 
> Sure, SUSBYSTEM=cuse will be exactly that value. :) I see no problem, if
> cuse adds SUBSYSTEM_FAKE, or whatever it would be.
> 
>> udev/hal whatever to use alt root for sys hierarchy or
>> just consider sysfs is not available?  I think this problem can be
>> settled between userland programs.
> 
> Exactly, we can do anything needed in userspace to support that. I see
> no problem doing that.
> 
> I just say, that we can _not_ allow to fake the SUBSYSTEM value for
> "cuse" or any other device. If the kernel wants to let "cuse" devices
> look like members of a specific subsystem, they have to appear in the
> class/bus list of that subsystem, and have to have the proper
> "susbsystem" link, not only a (wrong) environment key.
> 
> Userland enumerates devices by looking at class/bus directories for
> things like: "give me all soundcards", it expects a set of certain sysfs
> attributes to be present for a specific device, it reads the "subsystem"
> link, and so on. All that would break and make things "special"  and
> needlessly inconsistent.
> 
> So please drop that plan, of faking only a tiny part of
> driver-core/sysfs device parameters. Either we do it properly, or we
> don't do it, and leave it up to userspace to pick up a (consistent)
> "cuse" device, but use it as a device of whatever class.

Right, I was somehow fixated on sound devices having "sound" subsystem.
 That isn't necessary and if some special treatment is needed, we can
play with udev rules and so on.  Okay, will drop the kobject_uevent
changes and hotplug_info stuff.  It seems dev_info is all that's needed.

Thanks.

-- 
tejun