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

--

[ message continues ]

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;
 }
 ...
[ message continues ]

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
--

[ message continues ]

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
--

[ message continues ]

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

thanks,

greg k-h
--

[ message continues ]

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
--

[ message continues ]

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
--

[ message continues ]

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

--

[ message continues ]

Hello,


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
--

[ message continues ]

Sure, SUSBYSTEM=cuse will be exactly that value. :) I see no problem, if

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

--

[ message continues ]

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
--

[ message continues ]