| 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
| ||
|
Message-ID: <ZU3RlSmInnoXufxf@nanopsycho>
Date: Fri, 10 Nov 2023 07:45:41 +0100
From: Jiri Pirko <jiri@...nulli.us>
To: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
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
Fri, Nov 10, 2023 at 12:32:21AM CET, arkadiusz.kubalewski@...el.com wrote:
>>From: Jiri Pirko <jiri@...nulli.us>
>>Sent: Thursday, November 9, 2023 7:06 PM
>>
>>Thu, Nov 09, 2023 at 05:30:20PM CET, arkadiusz.kubalewski@...el.com wrote:
>>>>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.
>>
>>Or event better, as I suggested in the other patch reply, resolve this
>>internally in the driver registering things only when they are valid.
>>Much better then to hack anything in dpll core.
>>
>
>This approach seemed to me hacky, that is why started with coping the
>data.
>It is not about registering, rather about unregistering on driver
>unbind, which brakes things, and currently cannot be recovered in
>described case.
Sure it can. PF0 unbind-> internal notification-> PF1 unregisters all
related object. Very clean and simple.
>
>Thank you!
>Arkadiusz
>
>>
>>>
>>>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