| 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: <ZU0fj5y9mAvVzXuf@nanopsycho>
Date: Thu, 9 Nov 2023 19:06:07 +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
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.
>
>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