Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1

[Minfei Huang]

On 04/14/15 at 12:11P, Josh Poimboeuf wrote:
> On Tue, Apr 14, 2015 at 01:03:48PM +0800, Minfei Huang wrote:
> > On 04/13/15 at 11:57P, Josh Poimboeuf wrote:
> > > On Tue, Apr 14, 2015 at 08:26:29AM +0800, Minfei Huang wrote:
> > > > On 04/13/15 at 06:13P, Josh Poimboeuf wrote:
> > > > > On Sun, Apr 12, 2015 at 09:15:54PM +0800, Minfei Huang wrote:
> > > > > > For now, the kallsyms will only store the first (KSYM_NAME_LEN-1). The
> > > > > > kallsyms name is same for the function which first (KSYM_NAME_LEN-1) is
> > > > > > same, but the rest is not.
> > > > > > 
> > > > > > Then function will never be patched, although function name and address
> > > > > > are provided both. The reason caused this bug is livepatch cannt
> > > > > > recognize the function name.
> > > > > > 
> > > > > > Now, livepatch will verify the function name with first (KSYM_NAME_LEN-1)
> > > > > > and address, if provided. Once they are matched, we can confirm that the
> > > > > > patched function is found.
> > > > > 
> > > > > From scripts/kallsyms.c:
> > > > > 
> > > > > 	if (strlen(str) > KSYM_NAME_LEN) {
> > > > > 		fprintf(stderr, "Symbol %s too long for kallsyms (%zu vs %d).\n"
> > > > > 				"Please increase KSYM_NAME_LEN both in kernel and kallsyms.c\n",
> > > > > 			str, strlen(str), KSYM_NAME_LEN);
> > > > > 		return -1;
> > > > > 	}
> > > > > 
> > > > > So I think such a long symbol name wouldn't be added to the kallsyms
> > > > > database in the first place.
> > > > > 
> > > > 
> > > > Actually, kernel allows overlength function name to be used. Following
> > > > is my testing module.
> > > > 
> > > > We can got the address in /proc/kallsyms.
> > > > $ cat /proc/kallsyms | grep sysfs_print
> > > > ffffffffa0000000 t sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_pri  [sysfs_print]
> > > > ffffffffa0000010 t kobj_release [sysfs_print]
> > > > ffffffffa0000020 t sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_pri  [sysfs_print]
> > > > ffffffffa00004e0 b root_kobj    [sysfs_print]
> > > > ffffffffa0000200 d print_ktype  [sysfs_print]
> > > > ffffffffa00004a0 b print_kobj   [sysfs_print]
> > > > ffffffffa000004c t sys_print_exit   [sysfs_print]
> > > > ffffffffa0000144 r __func__.14514   [sysfs_print]
> > > > ffffffffa0000230 d kobj_attrs   [sysfs_print]
> > > > ffffffffa0000240 d sys_print_kobj_attr  [sysfs_print]
> > > > ffffffffa0000260 d __this_module    [sysfs_print]
> > > > ffffffffa000004c t cleanup_module   [sysfs_print]
> > > > 
> > > > Code:
> > > > 
> > > > static ssize_t sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_store(struct kobject *kobj, s
> > > >         const char *buf, size_t count)
> > > > {
> > > >     return count;
> > > > }
> > > > 
> > > > static ssize_t sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_show(struct kobject *kobj,
> > > >                 struct kobj_attribute *attr, char *buf)
> > > > {
> > > >     return snprintf(buf, PAGE_SIZE-1, "%s\n", "This is printed by module");
> > > > }
> > > > 
> > > > static struct kobj_attribute sys_print_kobj_attr = __ATTR_RW(sys_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_print_p
> > > > static struct attribute *kobj_attrs[] = {
> > > >     &sys_print_kobj_attr.attr,
> > > >     NULL
> > > > };
> > > > 
> > > 
> > > Hm, this seems like a kallsyms bug.  IMO it should either fail the build
> > > or omit the symbol from the kallsyms db.  Truncating it seems dangerous
> > > and counterintuitive.
> > > 
> > 
> > Kallsyms will record all of the function name, without truncating it.
> > But the kallsyms will return the truncated function name which is max to
> > 127.
> >
> > > But regardless I really don't see a good reason to encourage this kind
> > > of insanity in the livepatch code.
> > > 
> > 
> > Yes, the above code is terrible, but we cannt stop user composing like
> > that.
> > 
> > Once the function name is like above, user will never have chance to use
> > livepatch.
> 
> Again, this seems like a kallsyms bug.  Fix the bug and the real world
> need for this patch set goes away.  The user will be forced to either
> shorten their function name or increase KSYM_NAME_LEN.
> 

kallsyms bug? I donot think increasing the KSYM_NAME_LEN is a good idea.

For end user, they may know litter about restriction of kallsyms and
livepatch. How can they know the restriction that function name is
limited to 127?

It is significant that livepatch supports to running specified kernel
and extra module, not only for deliverying kernel.

Thanks
Minfei

> -- 
> Josh
> --
> To unsubscribe from this list: send the line "unsubscribe live-patching" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/