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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <DM6PR11MB46571D4C776B0B1888F943569BAFA@DM6PR11MB4657.namprd11.prod.outlook.com>
Date: Thu, 9 Nov 2023 16:30:20 +0000
From: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
To: Jiri Pirko <jiri@...nulli.us>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"vadim.fedorenko@...ux.dev" <vadim.fedorenko@...ux.dev>, "Michalik, Michal"
	<michal.michalik@...el.com>, "Olech, Milena" <milena.olech@...el.com>,
	"pabeni@...hat.com" <pabeni@...hat.com>, "kuba@...nel.org" <kuba@...nel.org>
Subject: RE: [PATCH net 2/3] dpll: fix pin dump crash for rebound module

>From: Jiri Pirko <jiri@...nulli.us>
>Sent: Thursday, November 9, 2023 2:19 PM
>
>Thu, Nov 09, 2023 at 01:20:48PM CET, arkadiusz.kubalewski@...el.com wrote:
>>>From: Jiri Pirko <jiri@...nulli.us>
>>>Sent: Wednesday, November 8, 2023 3:30 PM
>>>
>>>Wed, Nov 08, 2023 at 11:32:25AM CET, arkadiusz.kubalewski@...el.com
>>>wrote:
>>>>When a kernel module is unbound but the pin resources were not entirely
>>>>freed (other kernel module instance have had kept the reference to that
>>>>pin), and kernel module is again bound, the pin properties would not be
>>>>updated (the properties are only assigned when memory for the pin is
>>>>allocated), prop pointer still points to the kernel module memory of
>>>>the kernel module which was deallocated on the unbind.
>>>>
>>>>If the pin dump is invoked in this state, the result is a kernel crash.
>>>>Prevent the crash by storing persistent pin properties in dpll
>>>>subsystem,
>>>>copy the content from the kernel module when pin is allocated, instead
>>>>of
>>>>using memory of the kernel module.
>>>>
>>>>Fixes: 9431063ad323 ("dpll: core: Add DPLL framework base functions")
>>>>Fixes: 9d71b54b65b1 ("dpll: netlink: Add DPLL framework base functions")
>>>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
>>>>---
>>>> drivers/dpll/dpll_core.c    |  4 ++--
>>>> drivers/dpll/dpll_core.h    |  4 ++--
>>>> drivers/dpll/dpll_netlink.c | 28 ++++++++++++++--------------
>>>> 3 files changed, 18 insertions(+), 18 deletions(-)
>>>>
>>>>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>>>>index 3568149b9562..4077b562ba3b 100644
>>>>--- a/drivers/dpll/dpll_core.c
>>>>+++ b/drivers/dpll/dpll_core.c
>>>>@@ -442,7 +442,7 @@ dpll_pin_alloc(u64 clock_id, u32 pin_idx, struct
>>>>module *module,
>>>> 		ret = -EINVAL;
>>>> 		goto err;
>>>> 	}
>>>>-	pin->prop = prop;
>>>>+	memcpy(&pin->prop, prop, sizeof(pin->prop));
>>>
>>>Odd, you don't care about the pointer within this structure?
>>>
>>
>>Well, true. Need a fix.
>>Wondering if copying idea is better than just assigning prop pointer on
>>each call to dpll_pin_get(..) function (when pin already exists)?
>
>Not sure what do you mean. Examples please.
>

Sure,

Basically this change:

diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
index ae884b92d68c..06b72d5877c3 100644
--- a/drivers/dpll/dpll_core.c
+++ b/drivers/dpll/dpll_core.c
@@ -483,6 +483,7 @@ dpll_pin_get(u64 clock_id, u32 pin_idx, struct module *module,
                    pos->pin_idx == pin_idx &&
                    pos->module == module) {
                        ret = pos;
+                       pos->prop = prop;
                        refcount_inc(&ret->refcount);
                        break;
                }

would replace whole of this patch changes, although seems a bit hacky.

Thank you!
Arkadiusz

>
>>
>>Thank you!
>>Arkadiusz
>>
>>>
>>>> 	refcount_set(&pin->refcount, 1);
>>>> 	xa_init_flags(&pin->dpll_refs, XA_FLAGS_ALLOC);
>>>> 	xa_init_flags(&pin->parent_refs, XA_FLAGS_ALLOC);
>>>>@@ -634,7 +634,7 @@ int dpll_pin_on_pin_register(struct dpll_pin
>>>>*parent,
>>>>struct dpll_pin *pin,
>>>> 	unsigned long i, stop;
>>>> 	int ret;
>>>>
>>>>-	if (WARN_ON(parent->prop->type != DPLL_PIN_TYPE_MUX))
>>>>+	if (WARN_ON(parent->prop.type != DPLL_PIN_TYPE_MUX))
>>>> 		return -EINVAL;
>>>>
>>>> 	if (WARN_ON(!ops) ||
>>>>diff --git a/drivers/dpll/dpll_core.h b/drivers/dpll/dpll_core.h
>>>>index 5585873c5c1b..717f715015c7 100644
>>>>--- a/drivers/dpll/dpll_core.h
>>>>+++ b/drivers/dpll/dpll_core.h
>>>>@@ -44,7 +44,7 @@ struct dpll_device {
>>>>  * @module:		module of creator
>>>>  * @dpll_refs:		hold referencees to dplls pin was registered
>>>>with
>>>>  * @parent_refs:	hold references to parent pins pin was registered
>>>>with
>>>>- * @prop:		pointer to pin properties given by registerer
>>>>+ * @prop:		pin properties copied from the registerer
>>>>  * @rclk_dev_name:	holds name of device when pin can recover clock
>>>>from it
>>>>  * @refcount:		refcount
>>>>  **/
>>>>@@ -55,7 +55,7 @@ struct dpll_pin {
>>>> 	struct module *module;
>>>> 	struct xarray dpll_refs;
>>>> 	struct xarray parent_refs;
>>>>-	const struct dpll_pin_properties *prop;
>>>>+	struct dpll_pin_properties prop;
>>>> 	refcount_t refcount;
>>>> };
>>>>
>>>>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>>>>index 93fc6c4b8a78..963bbbbe6660 100644
>>>>--- a/drivers/dpll/dpll_netlink.c
>>>>+++ b/drivers/dpll/dpll_netlink.c
>>>>@@ -278,17 +278,17 @@ dpll_msg_add_pin_freq(struct sk_buff *msg, struct
>>>>dpll_pin *pin,
>>>> 	if (nla_put_64bit(msg, DPLL_A_PIN_FREQUENCY, sizeof(freq), &freq,
>>>> 			  DPLL_A_PIN_PAD))
>>>> 		return -EMSGSIZE;
>>>>-	for (fs = 0; fs < pin->prop->freq_supported_num; fs++) {
>>>>+	for (fs = 0; fs < pin->prop.freq_supported_num; fs++) {
>>>> 		nest = nla_nest_start(msg, DPLL_A_PIN_FREQUENCY_SUPPORTED);
>>>> 		if (!nest)
>>>> 			return -EMSGSIZE;
>>>>-		freq = pin->prop->freq_supported[fs].min;
>>>>+		freq = pin->prop.freq_supported[fs].min;
>>>> 		if (nla_put_64bit(msg, DPLL_A_PIN_FREQUENCY_MIN, sizeof(freq),
>>>> 				  &freq, DPLL_A_PIN_PAD)) {
>>>> 			nla_nest_cancel(msg, nest);
>>>> 			return -EMSGSIZE;
>>>> 		}
>>>>-		freq = pin->prop->freq_supported[fs].max;
>>>>+		freq = pin->prop.freq_supported[fs].max;
>>>> 		if (nla_put_64bit(msg, DPLL_A_PIN_FREQUENCY_MAX, sizeof(freq),
>>>> 				  &freq, DPLL_A_PIN_PAD)) {
>>>> 			nla_nest_cancel(msg, nest);
>>>>@@ -304,9 +304,9 @@ static bool dpll_pin_is_freq_supported(struct
>>>>dpll_pin
>>>>*pin, u32 freq)
>>>> {
>>>> 	int fs;
>>>>
>>>>-	for (fs = 0; fs < pin->prop->freq_supported_num; fs++)
>>>>-		if (freq >= pin->prop->freq_supported[fs].min &&
>>>>-		    freq <= pin->prop->freq_supported[fs].max)
>>>>+	for (fs = 0; fs < pin->prop.freq_supported_num; fs++)
>>>>+		if (freq >= pin->prop.freq_supported[fs].min &&
>>>>+		    freq <= pin->prop.freq_supported[fs].max)
>>>> 			return true;
>>>> 	return false;
>>>> }
>>>>@@ -403,7 +403,7 @@ static int
>>>> dpll_cmd_pin_get_one(struct sk_buff *msg, struct dpll_pin *pin,
>>>> 		     struct netlink_ext_ack *extack)
>>>> {
>>>>-	const struct dpll_pin_properties *prop = pin->prop;
>>>>+	const struct dpll_pin_properties *prop = &pin->prop;
>>>> 	struct dpll_pin_ref *ref;
>>>> 	int ret;
>>>>
>>>>@@ -696,7 +696,7 @@ dpll_pin_on_pin_state_set(struct dpll_pin *pin, u32
>>>>parent_idx,
>>>> 	int ret;
>>>>
>>>> 	if (!(DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE &
>>>>-	      pin->prop->capabilities)) {
>>>>+	      pin->prop.capabilities)) {
>>>> 		NL_SET_ERR_MSG(extack, "state changing is not allowed");
>>>> 		return -EOPNOTSUPP;
>>>> 	}
>>>>@@ -732,7 +732,7 @@ dpll_pin_state_set(struct dpll_device *dpll, struct
>>>>dpll_pin *pin,
>>>> 	int ret;
>>>>
>>>> 	if (!(DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE &
>>>>-	      pin->prop->capabilities)) {
>>>>+	      pin->prop.capabilities)) {
>>>> 		NL_SET_ERR_MSG(extack, "state changing is not allowed");
>>>> 		return -EOPNOTSUPP;
>>>> 	}
>>>>@@ -759,7 +759,7 @@ dpll_pin_prio_set(struct dpll_device *dpll, struct
>>>>dpll_pin *pin,
>>>> 	int ret;
>>>>
>>>> 	if (!(DPLL_PIN_CAPABILITIES_PRIORITY_CAN_CHANGE &
>>>>-	      pin->prop->capabilities)) {
>>>>+	      pin->prop.capabilities)) {
>>>> 		NL_SET_ERR_MSG(extack, "prio changing is not allowed");
>>>> 		return -EOPNOTSUPP;
>>>> 	}
>>>>@@ -787,7 +787,7 @@ dpll_pin_direction_set(struct dpll_pin *pin, struct
>>>>dpll_device *dpll,
>>>> 	int ret;
>>>>
>>>> 	if (!(DPLL_PIN_CAPABILITIES_DIRECTION_CAN_CHANGE &
>>>>-	      pin->prop->capabilities)) {
>>>>+	      pin->prop.capabilities)) {
>>>> 		NL_SET_ERR_MSG(extack, "direction changing is not allowed");
>>>> 		return -EOPNOTSUPP;
>>>> 	}
>>>>@@ -817,8 +817,8 @@ dpll_pin_phase_adj_set(struct dpll_pin *pin, struct
>>>>nlattr *phase_adj_attr,
>>>> 	int ret;
>>>>
>>>> 	phase_adj = nla_get_s32(phase_adj_attr);
>>>>-	if (phase_adj > pin->prop->phase_range.max ||
>>>>-	    phase_adj < pin->prop->phase_range.min) {
>>>>+	if (phase_adj > pin->prop.phase_range.max ||
>>>>+	    phase_adj < pin->prop.phase_range.min) {
>>>> 		NL_SET_ERR_MSG_ATTR(extack, phase_adj_attr,
>>>> 				    "phase adjust value not supported");
>>>> 		return -EINVAL;
>>>>@@ -999,7 +999,7 @@ dpll_pin_find(u64 clock_id, struct nlattr
>>>>*mod_name_attr,
>>>> 	unsigned long i;
>>>>
>>>> 	xa_for_each_marked(&dpll_pin_xa, i, pin, DPLL_REGISTERED) {
>>>>-		prop = pin->prop;
>>>>+		prop = &pin->prop;
>>>> 		cid_match = clock_id ? pin->clock_id == clock_id : true;
>>>> 		mod_match = mod_name_attr && module_name(pin->module) ?
>>>> 			!nla_strcmp(mod_name_attr,
>>>>--
>>>>2.38.1
>>>>
>>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ