netdev - RE: [PATCH net 3/3] dpll: fix register pin with unregistered parent pin

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] [thread-next>] [day] [month] [year] [list]

Date: Thu, 9 Nov 2023 16:13:48 +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 3/3] dpll: fix register pin with unregistered parent
 pin

>From: Jiri Pirko <jiri@...nulli.us>
>Sent: Thursday, November 9, 2023 2:20 PM
>
>Thu, Nov 09, 2023 at 10:59:04AM CET, arkadiusz.kubalewski@...el.com wrote:
>>>From: Jiri Pirko <jiri@...nulli.us>
>>>Sent: Wednesday, November 8, 2023 4:08 PM
>>>
>>>Wed, Nov 08, 2023 at 11:32:26AM CET, arkadiusz.kubalewski@...el.com
>>>wrote:
>>>>In case of multiple kernel module instances using the same dpll device:
>>>>if only one registers dpll device, then only that one can register
>>>
>>>They why you don't register in multiple instances? See mlx5 for a
>>>reference.
>>>
>>
>>Every registration requires ops, but for our case only PF0 is able to
>
>What makes PF0 so special? Smell like broken FW design... Care to fix
>it?
>

Well, from my perspective FW design it is.
AFAIR this single point of control is somehow related to HW design and
security requirements back when it was designed.. Don't think this would
be doable anytime soon (if doable at all).

Thank you!
Arkadiusz

>
>>control dpll pins and device, thus only this can provide ops.
>>Basically without PF0, dpll is not able to be controlled, as well
>>as directly connected pins.
>>
>>>
>>>>directly connected pins with a dpll device. If unregistered parent
>>>>determines if the muxed pin can be register with it or not, it forces
>>>>serialized driver load order - first the driver instance which
>>>>registers the direct pins needs to be loaded, then the other instances
>>>>could register muxed type pins.
>>>>
>>>>Allow registration of a pin with a parent even if the parent was not
>>>>yet registered, thus allow ability for unserialized driver instance
>>>
>>>Weird.
>>>
>>
>>Yeah, this is issue only for MUX/parent pin part, couldn't find better
>>way, but it doesn't seem to break things around..
>>
>>Thank you!
>>Arkadiusz
>>
>>>
>>>>load order.
>>>>Do not WARN_ON notification for unregistered pin, which can be invoked
>>>>for described case, instead just return error.
>>>>
>>>>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_netlink.c | 2 +-
>>>> 2 files changed, 1 insertion(+), 5 deletions(-)
>>>>
>>>>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c index
>>>>4077b562ba3b..ae884b92d68c 100644
>>>>--- a/drivers/dpll/dpll_core.c
>>>>+++ b/drivers/dpll/dpll_core.c
>>>>@@ -28,8 +28,6 @@ static u32 dpll_xa_id;
>>>> 	WARN_ON_ONCE(!xa_get_mark(&dpll_device_xa, (d)->id, DPLL_REGISTERED))
>>>> #define ASSERT_DPLL_NOT_REGISTERED(d)	\
>>>> 	WARN_ON_ONCE(xa_get_mark(&dpll_device_xa, (d)->id, DPLL_REGISTERED))
>>>>-#define ASSERT_PIN_REGISTERED(p)	\
>>>>-	WARN_ON_ONCE(!xa_get_mark(&dpll_pin_xa, (p)->id, DPLL_REGISTERED))
>>>>
>>>> struct dpll_device_registration {
>>>> 	struct list_head list;
>>>>@@ -641,8 +639,6 @@ int dpll_pin_on_pin_register(struct dpll_pin
>>>>*parent,
>>>struct dpll_pin *pin,
>>>> 	    WARN_ON(!ops->state_on_pin_get) ||
>>>> 	    WARN_ON(!ops->direction_get))
>>>> 		return -EINVAL;
>>>>-	if (ASSERT_PIN_REGISTERED(parent))
>>>>-		return -EINVAL;
>>>>
>>>> 	mutex_lock(&dpll_lock);
>>>> 	ret = dpll_xa_ref_pin_add(&pin->parent_refs, parent, ops, priv); diff
>>>>--git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c index
>>>>963bbbbe6660..ff430f43304f 100644
>>>>--- a/drivers/dpll/dpll_netlink.c
>>>>+++ b/drivers/dpll/dpll_netlink.c
>>>>@@ -558,7 +558,7 @@ dpll_pin_event_send(enum dpll_cmd event, struct
>>>>dpll_pin *pin)
>>>> 	int ret = -ENOMEM;
>>>> 	void *hdr;
>>>>
>>>>-	if (WARN_ON(!xa_get_mark(&dpll_pin_xa, pin->id, DPLL_REGISTERED)))
>>>>+	if (!xa_get_mark(&dpll_pin_xa, pin->id, DPLL_REGISTERED))
>>>> 		return -ENODEV;
>>>>
>>>> 	msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
>>>>--
>>>>2.38.1
>>>>