We need to use the _safe version of list_for_each_entry() because we
are freeing the iterator.
Signed-off-by: Dan Carpenter <[email protected]>
diff --git a/drivers/rapidio/rio.c b/drivers/rapidio/rio.c
index f4f30af..84ac64a 100644
--- a/drivers/rapidio/rio.c
+++ b/drivers/rapidio/rio.c
@@ -1701,7 +1701,7 @@ EXPORT_SYMBOL_GPL(rio_register_scan);
int rio_unregister_scan(int mport_id, struct rio_scan *scan_ops)
{
struct rio_mport *port;
- struct rio_scan_node *scan;
+ struct rio_scan_node *scan, *tmp;
pr_debug("RIO: %s for mport_id=%d\n", __func__, mport_id);
@@ -1715,7 +1715,7 @@ int rio_unregister_scan(int mport_id, struct rio_scan *scan_ops)
(mport_id == RIO_MPORT_ANY && port->nscan == scan_ops))
port->nscan = NULL;
- list_for_each_entry(scan, &rio_scans, node)
+ list_for_each_entry_safe(scan, tmp, &rio_scans, node)
if (scan->mport_id == mport_id) {
list_del(&scan->node);
kfree(scan);
On 05/07/13 16:02, Dan Carpenter wrote:
> We need to use the _safe version of list_for_each_entry() because we
> are freeing the iterator.
>
> Signed-off-by: Dan Carpenter <[email protected]>
>
> diff --git a/drivers/rapidio/rio.c b/drivers/rapidio/rio.c
> index f4f30af..84ac64a 100644
> --- a/drivers/rapidio/rio.c
> +++ b/drivers/rapidio/rio.c
> @@ -1701,7 +1701,7 @@ EXPORT_SYMBOL_GPL(rio_register_scan);
> int rio_unregister_scan(int mport_id, struct rio_scan *scan_ops)
> {
> struct rio_mport *port;
> - struct rio_scan_node *scan;
> + struct rio_scan_node *scan, *tmp;
>
> pr_debug("RIO: %s for mport_id=%d\n", __func__, mport_id);
>
> @@ -1715,7 +1715,7 @@ int rio_unregister_scan(int mport_id, struct rio_scan *scan_ops)
> (mport_id == RIO_MPORT_ANY && port->nscan == scan_ops))
> port->nscan = NULL;
>
> - list_for_each_entry(scan, &rio_scans, node)
> + list_for_each_entry_safe(scan, tmp, &rio_scans, node)
> if (scan->mport_id == mport_id) {
> list_del(&scan->node);
> kfree(scan);
It looks like an mport_id can only be assigned to one scan entry (see
rio_register_scan), so you can use list_for_each_entry and break; after
the kfree(scan); instead.
~Ryan
On Fri, Jul 05, 2013 at 05:06:14PM +1000, Ryan Mallon wrote:
> On 05/07/13 16:02, Dan Carpenter wrote:
> It looks like an mport_id can only be assigned to one scan entry (see
> rio_register_scan), so you can use list_for_each_entry and break; after
> the kfree(scan); instead.
Yeah. You're right. I'll resend.
regards,
dan carpenter
> From: Dan Carpenter [mailto:[email protected]]
> Sent: Friday, July 05, 2013 3:19 AM
> To: Ryan Mallon
> Cc: Matt Porter; Bounine, Alexandre; [email protected];
> [email protected]
> Subject: Re: [patch] rapidio: use after free in unregister function
>
> On Fri, Jul 05, 2013 at 05:06:14PM +1000, Ryan Mallon wrote:
> > On 05/07/13 16:02, Dan Carpenter wrote:
> > It looks like an mport_id can only be assigned to one scan entry (see
> > rio_register_scan), so you can use list_for_each_entry and break;
> after
> > the kfree(scan); instead.
>
> Yeah. You're right. I'll resend.
>
> regards,
> dan carpenter
Thank you for catching it. Missed it because we have only one enumerator so far.
Alex.
We're freeing the list iterator so we can't move to the next entry.
Since there is only one matching mport_id, we can just break after
finding it.
Signed-off-by: Dan Carpenter <[email protected]>
---
v2: cleaner fix than v1
diff --git a/drivers/rapidio/rio.c b/drivers/rapidio/rio.c
index f4f30af..2e8a20c 100644
--- a/drivers/rapidio/rio.c
+++ b/drivers/rapidio/rio.c
@@ -1715,11 +1715,13 @@ int rio_unregister_scan(int mport_id, struct rio_scan *scan_ops)
(mport_id == RIO_MPORT_ANY && port->nscan == scan_ops))
port->nscan = NULL;
- list_for_each_entry(scan, &rio_scans, node)
+ list_for_each_entry(scan, &rio_scans, node) {
if (scan->mport_id == mport_id) {
list_del(&scan->node);
kfree(scan);
+ break;
}
+ }
mutex_unlock(&rio_mport_list_lock);
On 06/07/13 06:39, Dan Carpenter wrote:
> We're freeing the list iterator so we can't move to the next entry.
> Since there is only one matching mport_id, we can just break after
> finding it.
>
> Signed-off-by: Dan Carpenter <[email protected]>
> ---
> v2: cleaner fix than v1
>
> diff --git a/drivers/rapidio/rio.c b/drivers/rapidio/rio.c
> index f4f30af..2e8a20c 100644
> --- a/drivers/rapidio/rio.c
> +++ b/drivers/rapidio/rio.c
> @@ -1715,11 +1715,13 @@ int rio_unregister_scan(int mport_id, struct rio_scan *scan_ops)
> (mport_id == RIO_MPORT_ANY && port->nscan == scan_ops))
> port->nscan = NULL;
>
> - list_for_each_entry(scan, &rio_scans, node)
> + list_for_each_entry(scan, &rio_scans, node) {
> if (scan->mport_id == mport_id) {
> list_del(&scan->node);
> kfree(scan);
> + break;
> }
> + }
>
> mutex_unlock(&rio_mport_list_lock);
>
Reviewed-by: Ryan Mallon <[email protected]>
On Friday, July 05, 2013 4:39 PM, Dan Carpenter wrote:
> We're freeing the list iterator so we can't move to the next entry.
> Since there is only one matching mport_id, we can just break after
> finding it.
>
> Signed-off-by: Dan Carpenter <[email protected]>
> ---
> v2: cleaner fix than v1
>
> diff --git a/drivers/rapidio/rio.c b/drivers/rapidio/rio.c
> index f4f30af..2e8a20c 100644
> --- a/drivers/rapidio/rio.c
> +++ b/drivers/rapidio/rio.c
> @@ -1715,11 +1715,13 @@ int rio_unregister_scan(int mport_id, struct
> rio_scan *scan_ops)
> (mport_id == RIO_MPORT_ANY && port->nscan == scan_ops))
> port->nscan = NULL;
>
> - list_for_each_entry(scan, &rio_scans, node)
> + list_for_each_entry(scan, &rio_scans, node) {
> if (scan->mport_id == mport_id) {
> list_del(&scan->node);
> kfree(scan);
> + break;
> }
> + }
>
> mutex_unlock(&rio_mport_list_lock);
>
Acked-by: Alexandre Bounine <[email protected]>