diff mbox series

[RFC,1/2] of: overlay: add whitelist

Message ID 1511816284-12145-2-git-send-email-atull@kernel.org
State Changes Requested, archived
Headers show
Series of: Add whitelist | expand

Commit Message

Alan Tull Nov. 27, 2017, 8:58 p.m. UTC
Add simple whitelist.  When an overlay is submitted, if any target in
the overlay is not in the whitelist, the overlay is rejected.  Drivers
that support dynamic configuration can register their device node with:

  int of_add_whitelist_node(struct device_node *np)

and remove themselves with:

  void of_remove_whitelist_node(struct device_node *np)

Signed-off-by: Alan Tull <atull@kernel.org>
---
 drivers/of/overlay.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of.h   | 12 +++++++++
 2 files changed, 85 insertions(+)

Comments

Rob Herring (Arm) Nov. 28, 2017, 3:15 p.m. UTC | #1
On Mon, Nov 27, 2017 at 02:58:03PM -0600, Alan Tull wrote:
> Add simple whitelist.  When an overlay is submitted, if any target in
> the overlay is not in the whitelist, the overlay is rejected.  Drivers
> that support dynamic configuration can register their device node with:
> 
>   int of_add_whitelist_node(struct device_node *np)
> 
> and remove themselves with:
> 
>   void of_remove_whitelist_node(struct device_node *np)

I think these should be named for what they do, not how it is 
implemented.

> 
> Signed-off-by: Alan Tull <atull@kernel.org>
> ---
>  drivers/of/overlay.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of.h   | 12 +++++++++
>  2 files changed, 85 insertions(+)
> 
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index c150abb..5f952a1 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -21,6 +21,7 @@
>  #include <linux/slab.h>
>  #include <linux/err.h>
>  #include <linux/idr.h>
> +#include <linux/spinlock.h>
>  
>  #include "of_private.h"
>  
> @@ -646,6 +647,74 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
>  	kfree(ovcs);
>  }
>  
> +/* lock for adding/removing device nodes to the whitelist */
> +static spinlock_t whitelist_lock;
> +
> +static struct list_head whitelist_list = LIST_HEAD_INIT(whitelist_list);
> +
> +struct dt_overlay_whitelist {
> +	struct device_node *np;
> +	struct list_head node;
> +};

Can't we just add a flags bit in device_node.flags? That would be much 
simpler.

> +
> +int of_add_whitelist_node(struct device_node *np)
> +{
> +	unsigned long flags;
> +	struct dt_overlay_whitelist *wln;
> +
> +	wln = kzalloc(sizeof(*wln), GFP_KERNEL);
> +	if (!wln)
> +		return -ENOMEM;
> +
> +	wln->np = np;
> +
> +	spin_lock_irqsave(&whitelist_lock, flags);
> +	list_add(&wln->node, &whitelist_list);
> +	spin_unlock_irqrestore(&whitelist_lock, flags);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_add_whitelist_node);
> +
> +void of_remove_whitelist_node(struct device_node *np)
> +{
> +	struct dt_overlay_whitelist *wln;
> +	unsigned long flags;
> +
> +	list_for_each_entry(wln, &whitelist_list, node) {
> +		if (np == wln->np) {
> +			spin_lock_irqsave(&whitelist_lock, flags);
> +			list_del(&wln->node);
> +			spin_unlock_irqrestore(&whitelist_lock, flags);
> +			kfree(wln);
> +			return;
> +		}
> +	}
> +}
> +EXPORT_SYMBOL_GPL(of_remove_whitelist_node);
> +
> +static int of_check_whitelist(struct overlay_changeset *ovcs)
> +{
> +	struct dt_overlay_whitelist *wln;
> +	struct device_node *target;
> +	int i;
> +
> +	for (i = 0; i < ovcs->count; i++) {
> +		target = ovcs->fragments[i].target;
> +		if (!of_node_cmp(target->name, "__symbols__"))
> +			continue;
> +
> +		list_for_each_entry(wln, &whitelist_list, node)
> +			if (target == wln->np)
> +				break;
> +
> +		if (target != wln->np)
> +			return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * of_overlay_apply() - Create and apply an overlay changeset
>   * @tree:	Expanded overlay device tree
> @@ -717,6 +786,10 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id)
>  	if (ret)
>  		goto err_free_overlay_changeset;
>  
> +	ret = of_check_whitelist(ovcs);
> +	if (ret)
> +		goto err_free_overlay_changeset;

This will break you until the next patch and breaks any other users. I 
think this is now just the unittest as tilcdc overlay is getting 
removed.

You have to make this chunk the last patch in the series.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Tull Nov. 28, 2017, 7:26 p.m. UTC | #2
On Tue, Nov 28, 2017 at 9:15 AM, Rob Herring <robh@kernel.org> wrote:
> On Mon, Nov 27, 2017 at 02:58:03PM -0600, Alan Tull wrote:
>> Add simple whitelist.  When an overlay is submitted, if any target in
>> the overlay is not in the whitelist, the overlay is rejected.  Drivers
>> that support dynamic configuration can register their device node with:
>>
>>   int of_add_whitelist_node(struct device_node *np)
>>
>> and remove themselves with:
>>
>>   void of_remove_whitelist_node(struct device_node *np)
>
> I think these should be named for what they do, not how it is
> implemented.

Sure, such as of_node_overlay_enable and of_node_overlay_disable?

>
>>
>> Signed-off-by: Alan Tull <atull@kernel.org>
>> ---
>>  drivers/of/overlay.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/of.h   | 12 +++++++++
>>  2 files changed, 85 insertions(+)
>>
>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> index c150abb..5f952a1 100644
>> --- a/drivers/of/overlay.c
>> +++ b/drivers/of/overlay.c
>> @@ -21,6 +21,7 @@
>>  #include <linux/slab.h>
>>  #include <linux/err.h>
>>  #include <linux/idr.h>
>> +#include <linux/spinlock.h>
>>
>>  #include "of_private.h"
>>
>> @@ -646,6 +647,74 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
>>       kfree(ovcs);
>>  }
>>
>> +/* lock for adding/removing device nodes to the whitelist */
>> +static spinlock_t whitelist_lock;
>> +
>> +static struct list_head whitelist_list = LIST_HEAD_INIT(whitelist_list);
>> +
>> +struct dt_overlay_whitelist {
>> +     struct device_node *np;
>> +     struct list_head node;
>> +};
>
> Can't we just add a flags bit in device_node.flags? That would be much
> simpler.

Yes, much simpler.  Such as:

#define OF_OVERLAY_ENABLED     5 /* allow DT overlay targeting this node */

>
>> +
>> +int of_add_whitelist_node(struct device_node *np)
>> +{
>> +     unsigned long flags;
>> +     struct dt_overlay_whitelist *wln;
>> +
>> +     wln = kzalloc(sizeof(*wln), GFP_KERNEL);
>> +     if (!wln)
>> +             return -ENOMEM;
>> +
>> +     wln->np = np;
>> +
>> +     spin_lock_irqsave(&whitelist_lock, flags);
>> +     list_add(&wln->node, &whitelist_list);
>> +     spin_unlock_irqrestore(&whitelist_lock, flags);
>> +
>> +     return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(of_add_whitelist_node);
>> +
>> +void of_remove_whitelist_node(struct device_node *np)
>> +{
>> +     struct dt_overlay_whitelist *wln;
>> +     unsigned long flags;
>> +
>> +     list_for_each_entry(wln, &whitelist_list, node) {
>> +             if (np == wln->np) {
>> +                     spin_lock_irqsave(&whitelist_lock, flags);
>> +                     list_del(&wln->node);
>> +                     spin_unlock_irqrestore(&whitelist_lock, flags);
>> +                     kfree(wln);
>> +                     return;
>> +             }
>> +     }
>> +}
>> +EXPORT_SYMBOL_GPL(of_remove_whitelist_node);
>> +
>> +static int of_check_whitelist(struct overlay_changeset *ovcs)
>> +{
>> +     struct dt_overlay_whitelist *wln;
>> +     struct device_node *target;
>> +     int i;
>> +
>> +     for (i = 0; i < ovcs->count; i++) {
>> +             target = ovcs->fragments[i].target;
>> +             if (!of_node_cmp(target->name, "__symbols__"))
>> +                     continue;
>> +
>> +             list_for_each_entry(wln, &whitelist_list, node)
>> +                     if (target == wln->np)
>> +                             break;
>> +
>> +             if (target != wln->np)
>> +                     return -ENODEV;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>>  /**
>>   * of_overlay_apply() - Create and apply an overlay changeset
>>   * @tree:    Expanded overlay device tree
>> @@ -717,6 +786,10 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id)
>>       if (ret)
>>               goto err_free_overlay_changeset;
>>
>> +     ret = of_check_whitelist(ovcs);
>> +     if (ret)
>> +             goto err_free_overlay_changeset;
>
> This will break you until the next patch and breaks any other users. I
> think this is now just the unittest as tilcdc overlay is getting
> removed.
>
> You have to make this chunk the last patch in the series.

I'd rather squash the two patches.  In either case, the contents of
second patch are dependent on stuff in char-misc-testing today, so it
won't be able to apply yet on linux-next or anywhere else.

Thanks
Alan

>
> Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Frank Rowand Nov. 29, 2017, 9:25 a.m. UTC | #3
On 11/28/17 14:26, Alan Tull wrote:
> On Tue, Nov 28, 2017 at 9:15 AM, Rob Herring <robh@kernel.org> wrote:
>> On Mon, Nov 27, 2017 at 02:58:03PM -0600, Alan Tull wrote:
>>> Add simple whitelist.  When an overlay is submitted, if any target in
>>> the overlay is not in the whitelist, the overlay is rejected.  Drivers
>>> that support dynamic configuration can register their device node with:
>>>
>>>   int of_add_whitelist_node(struct device_node *np)
>>>
>>> and remove themselves with:
>>>
>>>   void of_remove_whitelist_node(struct device_node *np)
>>
>> I think these should be named for what they do, not how it is
>> implemented.
> 
> Sure, such as of_node_overlay_enable and of_node_overlay_disable?
of_allow_overlay_on_node(), of_disallow_overlay_on_node()?


> 
>>
>>>
>>> Signed-off-by: Alan Tull <atull@kernel.org>
>>> ---
>>>  drivers/of/overlay.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  include/linux/of.h   | 12 +++++++++
>>>  2 files changed, 85 insertions(+)
>>>
>>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>>> index c150abb..5f952a1 100644
>>> --- a/drivers/of/overlay.c
>>> +++ b/drivers/of/overlay.c
>>> @@ -21,6 +21,7 @@
>>>  #include <linux/slab.h>
>>>  #include <linux/err.h>
>>>  #include <linux/idr.h>
>>> +#include <linux/spinlock.h>
>>>
>>>  #include "of_private.h"
>>>
>>> @@ -646,6 +647,74 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
>>>       kfree(ovcs);
>>>  }
>>>
>>> +/* lock for adding/removing device nodes to the whitelist */
>>> +static spinlock_t whitelist_lock;
>>> +
>>> +static struct list_head whitelist_list = LIST_HEAD_INIT(whitelist_list);
>>> +
>>> +struct dt_overlay_whitelist {
>>> +     struct device_node *np;
>>> +     struct list_head node;
>>> +};
>>
>> Can't we just add a flags bit in device_node.flags? That would be much
>> simpler.
> 
> Yes, much simpler.  Such as:
> 
> #define OF_OVERLAY_ENABLED     5 /* allow DT overlay targeting this node */
> 
>>
>>> +
>>> +int of_add_whitelist_node(struct device_node *np)
>>> +{
>>> +     unsigned long flags;
>>> +     struct dt_overlay_whitelist *wln;
>>> +
>>> +     wln = kzalloc(sizeof(*wln), GFP_KERNEL);
>>> +     if (!wln)
>>> +             return -ENOMEM;
>>> +
>>> +     wln->np = np;
>>> +
>>> +     spin_lock_irqsave(&whitelist_lock, flags);
>>> +     list_add(&wln->node, &whitelist_list);
>>> +     spin_unlock_irqrestore(&whitelist_lock, flags);
>>> +
>>> +     return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(of_add_whitelist_node);
>>> +
>>> +void of_remove_whitelist_node(struct device_node *np)
>>> +{
>>> +     struct dt_overlay_whitelist *wln;
>>> +     unsigned long flags;
>>> +
>>> +     list_for_each_entry(wln, &whitelist_list, node) {
>>> +             if (np == wln->np) {
>>> +                     spin_lock_irqsave(&whitelist_lock, flags);
>>> +                     list_del(&wln->node);
>>> +                     spin_unlock_irqrestore(&whitelist_lock, flags);
>>> +                     kfree(wln);
>>> +                     return;
>>> +             }
>>> +     }
>>> +}
>>> +EXPORT_SYMBOL_GPL(of_remove_whitelist_node);
>>> +
>>> +static int of_check_whitelist(struct overlay_changeset *ovcs)
>>> +{
>>> +     struct dt_overlay_whitelist *wln;
>>> +     struct device_node *target;
>>> +     int i;
>>> +
>>> +     for (i = 0; i < ovcs->count; i++) {
>>> +             target = ovcs->fragments[i].target;
>>> +             if (!of_node_cmp(target->name, "__symbols__"))
>>> +                     continue;
>>> +
>>> +             list_for_each_entry(wln, &whitelist_list, node)
>>> +                     if (target == wln->np)
>>> +                             break;
>>> +
>>> +             if (target != wln->np)
>>> +                     return -ENODEV;
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>> +
>>>  /**
>>>   * of_overlay_apply() - Create and apply an overlay changeset
>>>   * @tree:    Expanded overlay device tree
>>> @@ -717,6 +786,10 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id)
>>>       if (ret)
>>>               goto err_free_overlay_changeset;
>>>
>>> +     ret = of_check_whitelist(ovcs);
>>> +     if (ret)
>>> +             goto err_free_overlay_changeset;
>>
>> This will break you until the next patch and breaks any other users. I
>> think this is now just the unittest as tilcdc overlay is getting
>> removed.
>>
>> You have to make this chunk the last patch in the series.
> 
> I'd rather squash the two patches.  In either case, the contents of
> second patch are dependent on stuff in char-misc-testing today, so it
> won't be able to apply yet on linux-next or anywhere else.
> 
> Thanks
> Alan
> 
>>
>> Rob
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox series

Patch

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index c150abb..5f952a1 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -21,6 +21,7 @@ 
 #include <linux/slab.h>
 #include <linux/err.h>
 #include <linux/idr.h>
+#include <linux/spinlock.h>
 
 #include "of_private.h"
 
@@ -646,6 +647,74 @@  static void free_overlay_changeset(struct overlay_changeset *ovcs)
 	kfree(ovcs);
 }
 
+/* lock for adding/removing device nodes to the whitelist */
+static spinlock_t whitelist_lock;
+
+static struct list_head whitelist_list = LIST_HEAD_INIT(whitelist_list);
+
+struct dt_overlay_whitelist {
+	struct device_node *np;
+	struct list_head node;
+};
+
+int of_add_whitelist_node(struct device_node *np)
+{
+	unsigned long flags;
+	struct dt_overlay_whitelist *wln;
+
+	wln = kzalloc(sizeof(*wln), GFP_KERNEL);
+	if (!wln)
+		return -ENOMEM;
+
+	wln->np = np;
+
+	spin_lock_irqsave(&whitelist_lock, flags);
+	list_add(&wln->node, &whitelist_list);
+	spin_unlock_irqrestore(&whitelist_lock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_add_whitelist_node);
+
+void of_remove_whitelist_node(struct device_node *np)
+{
+	struct dt_overlay_whitelist *wln;
+	unsigned long flags;
+
+	list_for_each_entry(wln, &whitelist_list, node) {
+		if (np == wln->np) {
+			spin_lock_irqsave(&whitelist_lock, flags);
+			list_del(&wln->node);
+			spin_unlock_irqrestore(&whitelist_lock, flags);
+			kfree(wln);
+			return;
+		}
+	}
+}
+EXPORT_SYMBOL_GPL(of_remove_whitelist_node);
+
+static int of_check_whitelist(struct overlay_changeset *ovcs)
+{
+	struct dt_overlay_whitelist *wln;
+	struct device_node *target;
+	int i;
+
+	for (i = 0; i < ovcs->count; i++) {
+		target = ovcs->fragments[i].target;
+		if (!of_node_cmp(target->name, "__symbols__"))
+			continue;
+
+		list_for_each_entry(wln, &whitelist_list, node)
+			if (target == wln->np)
+				break;
+
+		if (target != wln->np)
+			return -ENODEV;
+	}
+
+	return 0;
+}
+
 /**
  * of_overlay_apply() - Create and apply an overlay changeset
  * @tree:	Expanded overlay device tree
@@ -717,6 +786,10 @@  int of_overlay_apply(struct device_node *tree, int *ovcs_id)
 	if (ret)
 		goto err_free_overlay_changeset;
 
+	ret = of_check_whitelist(ovcs);
+	if (ret)
+		goto err_free_overlay_changeset;
+
 	ret = overlay_notify(ovcs, OF_OVERLAY_PRE_APPLY);
 	if (ret) {
 		pr_err("overlay changeset pre-apply notify error %d\n", ret);
diff --git a/include/linux/of.h b/include/linux/of.h
index d3dea1d..5bf652a1 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1364,6 +1364,9 @@  int of_overlay_remove_all(void);
 int of_overlay_notifier_register(struct notifier_block *nb);
 int of_overlay_notifier_unregister(struct notifier_block *nb);
 
+int of_add_whitelist_node(struct device_node *np);
+void of_remove_whitelist_node(struct device_node *np);
+
 #else
 
 static inline int of_overlay_apply(struct device_node *tree, int *ovcs_id)
@@ -1391,6 +1394,15 @@  static inline int of_overlay_notifier_unregister(struct notifier_block *nb)
 	return 0;
 }
 
+static inline int of_add_whitelist_node(struct device_node *np)
+{
+	return 0;
+}
+
+static inline void of_remove_whitelist_node(struct device_node *np)
+{
+}
+
 #endif
 
 #endif /* _LINUX_OF_H */