linux-kernel - Re: [PATCH 1/2] livepatch: introduce 'order' sysfs interface to klp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ZvLVSq7AQBFwVmsW@pathway.suse.cz>
Date: Tue, 24 Sep 2024 17:05:46 +0200
From: Petr Mladek <pmladek@...e.com>
To: Wardenjohn <zhangwarden@...il.com>
Cc: jpoimboe@...nel.org, mbenes@...e.cz, jikos@...nel.org,
	joe.lawrence@...hat.com, live-patching@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] livepatch: introduce 'order' sysfs interface to
 klp_patch

On Tue 2024-09-24 13:27:58, Petr Mladek wrote:
> On Fri 2024-09-20 17:04:03, Wardenjohn wrote:
> > This feature can provide livepatch patch order information.
> > With the order of sysfs interface of one klp_patch, we can
> > use patch order to find out which function of the patch is
> > now activate.
> > 
> > After the discussion, we decided that patch-level sysfs
> > interface is the only accaptable way to introduce this
> > information.
> > 
> > This feature is like:
> > cat /sys/kernel/livepatch/livepatch_1/order -> 1
> > means this livepatch_1 module is the 1st klp patch applied.
> > 
> > cat /sys/kernel/livepatch/livepatch_module/order -> N
> > means this lviepatch_module is the Nth klp patch applied
> > to the system.
> >
> > --- a/kernel/livepatch/transition.c
> > +++ b/kernel/livepatch/transition.c
> > @@ -46,6 +46,15 @@ EXPORT_SYMBOL(klp_sched_try_switch_key);
> >  
> >  #endif /* CONFIG_PREEMPT_DYNAMIC && CONFIG_HAVE_PREEMPT_DYNAMIC_CALL */
> >  
> > +static inline int klp_get_patch_order(struct klp_patch *patch)
> > +{
> > +	int order = 0;
> > +
> > +	klp_for_each_patch(patch)
> > +		order = order + 1;
> > +	return order;
> > +}
> 
> This does not work well. It uses the order on the stack when
> the livepatch is being loaded. It is not updated when any livepatch gets
> removed. It might create wrong values.
> 
> I have even tried to reproduce this:
> 
> 	# modprobe livepatch-sample
> 	# modprobe livepatch-shadow-fix1
> 	# cat /sys/kernel/livepatch/livepatch_sample/order
> 	1
> 	# cat /sys/kernel/livepatch/livepatch_shadow_fix1/order
> 	2
> 
> 	# echo 0 >/sys/kernel/livepatch/livepatch_sample/enabled
> 	# rmmod livepatch_sample
> 	# cat /sys/kernel/livepatch/livepatch_shadow_fix1/order
> 	2
> 
> 	# modprobe livepatch-sample
> 	# cat /sys/kernel/livepatch/livepatch_shadow_fix1/order
> 	2
> 	# cat /sys/kernel/livepatch/livepatch_sample/order
> 	2
> 
> BANG: The livepatches have the same order.
> 
> I suggest to replace this with a global load counter. Something like:

Wait! Miroslav asked me on chat about a possibility to use klp_mutex
in the sysfs order_show() callback. It is a good point.

If I get it correctly then we actually could take klp_mutex in
the sysfs callbacks associated with struct klp_patch, similar
to enabled_store().

It should be safe because the "final" kobject_put(&patch->kobj) is
called without klp_mutex, see klp_free_patch_finish(). It was
explicitly done this way to allow taking klp_mutex in
enabled_store().

Note that it was not safe to take klp_mutex in the sysfs callback
associated with struct klp_func. The "final" kobject_put(&func->kobj)
is called under klp_mutex, see __klp_free_funcs().


Back to the order_show(). We could take klp_mutex there => we do not
need to store the order in struct klp_patch. Instead, we could do
something like:

static ssize_t order_show(struct kobject *kobj,
			struct kobj_attribute *attr, char *buf)
{
	struct klp_patch *this_patch, *patch;
	int order;

	this_patch = container_of(kobj, struct klp_patch, kobj);

	mutex_lock(&klp_mutex);

	order = 0;
	klp_for_each_patch(patch) {
		order++;
		if (patch == this_patch)
			break;
	}

	mutex_unlock(&klp_mutex);

	return sysfs_emit(buf, "%d\n", order);
}

BTW: I would prefer to rename the attribute from "order" to "stack_order".
     IMHO, it would make the meaning more clear.

Best Regards,
Petr