| Message ID | 20231108103226.1168500-2-arkadiusz.kubalewski@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | dpll: fix unordered unbind/bind registerer issues | expand |
On 11/8/23 11:32, Arkadiusz Kubalewski wrote: > Disallow dump of unregistered parent pins, it is possible when parent > pin and dpll device registerer kernel module instance unbinds, and > other kernel module instances of the same dpll device have pins > registered with the parent pin. The user can invoke a pin-dump but as > the parent was unregistered, thus shall not be accessed by the > userspace, prevent that by checking if parent pin is still registered. > > Fixes: 9d71b54b65b1 ("dpll: netlink: Add DPLL framework base functions") > Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> > --- > drivers/dpll/dpll_netlink.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c > index a6dc3997bf5c..93fc6c4b8a78 100644 > --- a/drivers/dpll/dpll_netlink.c > +++ b/drivers/dpll/dpll_netlink.c > @@ -328,6 +328,13 @@ dpll_msg_add_pin_parents(struct sk_buff *msg, struct dpll_pin *pin, > void *parent_priv; > > ppin = ref->pin; > + /* > + * dump parent only if it is registered, thus prevent crash on > + * pin dump called when driver which registered the pin unbinds > + * and different instance registered pin on that parent pin > + */ > + if (!xa_get_mark(&dpll_pin_xa, ppin->id, DPLL_REGISTERED)) > + continue; What if unregister/unbind would happen right [here]? [here] > parent_priv = dpll_pin_on_dpll_priv(dpll_ref->dpll, ppin); > ret = ops->state_on_pin_get(pin, > dpll_pin_on_pin_priv(ppin, pin),
>From: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com> >Sent: Wednesday, November 8, 2023 12:36 PM > >On 11/8/23 11:32, Arkadiusz Kubalewski wrote: >> Disallow dump of unregistered parent pins, it is possible when parent >> pin and dpll device registerer kernel module instance unbinds, and >> other kernel module instances of the same dpll device have pins >> registered with the parent pin. The user can invoke a pin-dump but as >> the parent was unregistered, thus shall not be accessed by the >> userspace, prevent that by checking if parent pin is still registered. >> >> Fixes: 9d71b54b65b1 ("dpll: netlink: Add DPLL framework base functions") >> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >> --- >> drivers/dpll/dpll_netlink.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c >> index a6dc3997bf5c..93fc6c4b8a78 100644 >> --- a/drivers/dpll/dpll_netlink.c >> +++ b/drivers/dpll/dpll_netlink.c >> @@ -328,6 +328,13 @@ dpll_msg_add_pin_parents(struct sk_buff *msg, struct >dpll_pin *pin, >> void *parent_priv; >> >> ppin = ref->pin; >> + /* >> + * dump parent only if it is registered, thus prevent crash on >> + * pin dump called when driver which registered the pin unbinds >> + * and different instance registered pin on that parent pin >> + */ >> + if (!xa_get_mark(&dpll_pin_xa, ppin->id, DPLL_REGISTERED)) >> + continue; > >What if unregister/unbind would happen right [here]? >[here] There is a "global" mutex lock which guards the pin/dpll registration and all netlink requests. For netlink requests in this case it is acquired in the dpll_pin_pre_doit(..), while all the dpll subsystem interaction from kernel modules are guarded in dpll_core.c api functions with the same lock. So after all this use case is protected, just "higher" in the stack. Thank you! Arkadiusz > >> parent_priv = dpll_pin_on_dpll_priv(dpll_ref->dpll, ppin); >> ret = ops->state_on_pin_get(pin, >> dpll_pin_on_pin_priv(ppin, pin),
Wed, Nov 08, 2023 at 11:32:24AM CET, arkadiusz.kubalewski@intel.com wrote: >Disallow dump of unregistered parent pins, it is possible when parent >pin and dpll device registerer kernel module instance unbinds, and >other kernel module instances of the same dpll device have pins >registered with the parent pin. The user can invoke a pin-dump but as >the parent was unregistered, thus shall not be accessed by the >userspace, prevent that by checking if parent pin is still registered. > >Fixes: 9d71b54b65b1 ("dpll: netlink: Add DPLL framework base functions") >Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >--- > drivers/dpll/dpll_netlink.c | 7 +++++++ > 1 file changed, 7 insertions(+) > >diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c >index a6dc3997bf5c..93fc6c4b8a78 100644 >--- a/drivers/dpll/dpll_netlink.c >+++ b/drivers/dpll/dpll_netlink.c >@@ -328,6 +328,13 @@ dpll_msg_add_pin_parents(struct sk_buff *msg, struct dpll_pin *pin, > void *parent_priv; > > ppin = ref->pin; >+ /* >+ * dump parent only if it is registered, thus prevent crash on >+ * pin dump called when driver which registered the pin unbinds >+ * and different instance registered pin on that parent pin Read this sentence like 10 times, still don't get what you mean. Shouldn't comments be easy to understand? >+ */ >+ if (!xa_get_mark(&dpll_pin_xa, ppin->id, DPLL_REGISTERED)) >+ continue; > parent_priv = dpll_pin_on_dpll_priv(dpll_ref->dpll, ppin); > ret = ops->state_on_pin_get(pin, > dpll_pin_on_pin_priv(ppin, pin), >-- >2.38.1 >
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Wednesday, November 8, 2023 4:09 PM > >Wed, Nov 08, 2023 at 11:32:24AM CET, arkadiusz.kubalewski@intel.com wrote: >>Disallow dump of unregistered parent pins, it is possible when parent >>pin and dpll device registerer kernel module instance unbinds, and >>other kernel module instances of the same dpll device have pins >>registered with the parent pin. The user can invoke a pin-dump but as >>the parent was unregistered, thus shall not be accessed by the >>userspace, prevent that by checking if parent pin is still registered. >> >>Fixes: 9d71b54b65b1 ("dpll: netlink: Add DPLL framework base functions") >>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >>--- >> drivers/dpll/dpll_netlink.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c >>index a6dc3997bf5c..93fc6c4b8a78 100644 >>--- a/drivers/dpll/dpll_netlink.c >>+++ b/drivers/dpll/dpll_netlink.c >>@@ -328,6 +328,13 @@ dpll_msg_add_pin_parents(struct sk_buff *msg, struct >dpll_pin *pin, >> void *parent_priv; >> >> ppin = ref->pin; >>+ /* >>+ * dump parent only if it is registered, thus prevent crash on >>+ * pin dump called when driver which registered the pin unbinds >>+ * and different instance registered pin on that parent pin > >Read this sentence like 10 times, still don't get what you mean. >Shouldn't comments be easy to understand? > Hi, Hmm, wondering isn't it better to remove this comment at all? If you think it is needed I will rephrase it somehow.. Thank you! Arkadiusz > >>+ */ >>+ if (!xa_get_mark(&dpll_pin_xa, ppin->id, DPLL_REGISTERED)) >>+ continue; >> parent_priv = dpll_pin_on_dpll_priv(dpll_ref->dpll, ppin); >> ret = ops->state_on_pin_get(pin, >> dpll_pin_on_pin_priv(ppin, pin), >>-- >>2.38.1 >>
Thu, Nov 09, 2023 at 10:49:49AM CET, arkadiusz.kubalewski@intel.com wrote: >>From: Jiri Pirko <jiri@resnulli.us> >>Sent: Wednesday, November 8, 2023 4:09 PM >> >>Wed, Nov 08, 2023 at 11:32:24AM CET, arkadiusz.kubalewski@intel.com wrote: >>>Disallow dump of unregistered parent pins, it is possible when parent >>>pin and dpll device registerer kernel module instance unbinds, and >>>other kernel module instances of the same dpll device have pins >>>registered with the parent pin. The user can invoke a pin-dump but as >>>the parent was unregistered, thus shall not be accessed by the >>>userspace, prevent that by checking if parent pin is still registered. >>> >>>Fixes: 9d71b54b65b1 ("dpll: netlink: Add DPLL framework base functions") >>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >>>--- >>> drivers/dpll/dpll_netlink.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c >>>index a6dc3997bf5c..93fc6c4b8a78 100644 >>>--- a/drivers/dpll/dpll_netlink.c >>>+++ b/drivers/dpll/dpll_netlink.c >>>@@ -328,6 +328,13 @@ dpll_msg_add_pin_parents(struct sk_buff *msg, struct >>dpll_pin *pin, >>> void *parent_priv; >>> >>> ppin = ref->pin; >>>+ /* >>>+ * dump parent only if it is registered, thus prevent crash on >>>+ * pin dump called when driver which registered the pin unbinds >>>+ * and different instance registered pin on that parent pin >> >>Read this sentence like 10 times, still don't get what you mean. >>Shouldn't comments be easy to understand? >> > >Hi, > >Hmm, wondering isn't it better to remove this comment at all? >If you think it is needed I will rephrase it somehow.. I don't know if it is needed as I don't understand it :) Just remove it. > >Thank you! >Arkadiusz > >> >>>+ */ >>>+ if (!xa_get_mark(&dpll_pin_xa, ppin->id, DPLL_REGISTERED)) >>>+ continue; >>> parent_priv = dpll_pin_on_dpll_priv(dpll_ref->dpll, ppin); >>> ret = ops->state_on_pin_get(pin, >>> dpll_pin_on_pin_priv(ppin, pin), >>>-- >>>2.38.1 >>> >
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Thursday, November 9, 2023 2:18 PM > >Thu, Nov 09, 2023 at 10:49:49AM CET, arkadiusz.kubalewski@intel.com wrote: >>>From: Jiri Pirko <jiri@resnulli.us> >>>Sent: Wednesday, November 8, 2023 4:09 PM >>> >>>Wed, Nov 08, 2023 at 11:32:24AM CET, arkadiusz.kubalewski@intel.com >>>wrote: >>>>Disallow dump of unregistered parent pins, it is possible when parent >>>>pin and dpll device registerer kernel module instance unbinds, and >>>>other kernel module instances of the same dpll device have pins >>>>registered with the parent pin. The user can invoke a pin-dump but as >>>>the parent was unregistered, thus shall not be accessed by the >>>>userspace, prevent that by checking if parent pin is still registered. >>>> >>>>Fixes: 9d71b54b65b1 ("dpll: netlink: Add DPLL framework base functions") >>>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >>>>--- >>>> drivers/dpll/dpll_netlink.c | 7 +++++++ >>>> 1 file changed, 7 insertions(+) >>>> >>>>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c >>>>index a6dc3997bf5c..93fc6c4b8a78 100644 >>>>--- a/drivers/dpll/dpll_netlink.c >>>>+++ b/drivers/dpll/dpll_netlink.c >>>>@@ -328,6 +328,13 @@ dpll_msg_add_pin_parents(struct sk_buff *msg, >>>> struct dpll_pin *pin, >>>> void *parent_priv; >>>> >>>> ppin = ref->pin; >>>>+ /* >>>>+ * dump parent only if it is registered, thus prevent crash on >>>>+ * pin dump called when driver which registered the pin unbinds >>>>+ * and different instance registered pin on that parent pin >>> >>>Read this sentence like 10 times, still don't get what you mean. >>>Shouldn't comments be easy to understand? >>> >> >>Hi, >> >>Hmm, wondering isn't it better to remove this comment at all? >>If you think it is needed I will rephrase it somehow.. > >I don't know if it is needed as I don't understand it :) >Just remove it. > Sure, will do. Thank you! Arkadiusz > >> >>Thank you! >>Arkadiusz >> >>> >>>>+ */ >>>>+ if (!xa_get_mark(&dpll_pin_xa, ppin->id, DPLL_REGISTERED)) >>>>+ continue; >>>> parent_priv = dpll_pin_on_dpll_priv(dpll_ref->dpll, ppin); >>>> ret = ops->state_on_pin_get(pin, >>>> dpll_pin_on_pin_priv(ppin, pin), >>>>-- >>>>2.38.1 >>>> >>
diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c index a6dc3997bf5c..93fc6c4b8a78 100644 --- a/drivers/dpll/dpll_netlink.c +++ b/drivers/dpll/dpll_netlink.c @@ -328,6 +328,13 @@ dpll_msg_add_pin_parents(struct sk_buff *msg, struct dpll_pin *pin, void *parent_priv; ppin = ref->pin; + /* + * dump parent only if it is registered, thus prevent crash on + * pin dump called when driver which registered the pin unbinds + * and different instance registered pin on that parent pin + */ + if (!xa_get_mark(&dpll_pin_xa, ppin->id, DPLL_REGISTERED)) + continue; parent_priv = dpll_pin_on_dpll_priv(dpll_ref->dpll, ppin); ret = ops->state_on_pin_get(pin, dpll_pin_on_pin_priv(ppin, pin),
Disallow dump of unregistered parent pins, it is possible when parent pin and dpll device registerer kernel module instance unbinds, and other kernel module instances of the same dpll device have pins registered with the parent pin. The user can invoke a pin-dump but as the parent was unregistered, thus shall not be accessed by the userspace, prevent that by checking if parent pin is still registered. Fixes: 9d71b54b65b1 ("dpll: netlink: Add DPLL framework base functions") Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> --- drivers/dpll/dpll_netlink.c | 7 +++++++ 1 file changed, 7 insertions(+)