| Message ID | e77756aa-c0f1-44a8-bcd6-a46fe954b195@moroto.mountain (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Input: synaptics-rmi4 - fix use after free (more completely) | expand |
Hi Dan, Torsten, On 22.02.24 09:53, Dan Carpenter wrote: > From: Brad Spengler <spender@grsecurity.net> > > Grsecurity uses memory poisoning and detected a use after free in > rmi_unregister_function(). A fix for this was merged into the > grsecurity patchset. I detected the user after free later using static ^^^^ use after free > analysis and sent a partial fix as commit eb988e46da2e ("Input: > synaptics-rmi4 - fix use after free in rmi_unregister_function()"). > However, my patch meant that the grsecurity patchset didn't apply cleanly > and forced everyone to re-visit this issue. Indeed. We even noticed that your patch was incomplete back then but urgent matters kept us from following-up. Then came holidays and this simply went forgotten :( > > The bug is that we must not call device_del() until after calling > irq_dispose_mapping(). Unfortunately, this is only half the truth. We investigated this further and there's another bug that got introduced in commit 24d28e4f1271 ("Input: synaptics-rmi4 - convert irq distribution to irq_domain"). The IRQ domain has a UAF issue as well. I'll send the patch soon -- wanted to do so this week, but, again, more urgent matters interrupted this. > > Fixes: 24d28e4f1271 ("Input: synaptics-rmi4 - convert irq distribution to irq_domain") > Reported-by: Torsten Hilbrich <torsten.hilbrich@secunet.com> > Closes: https://lore.kernel.org/all/1932038e-2776-04ac-5fcd-b15bb3cd088d@secunet.com/ > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=215604 > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > drivers/input/rmi4/rmi_bus.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c > index 1b45b1d3077d..02acc81b9d3e 100644 > --- a/drivers/input/rmi4/rmi_bus.c > +++ b/drivers/input/rmi4/rmi_bus.c > @@ -275,12 +275,11 @@ void rmi_unregister_function(struct rmi_function *fn) > rmi_dbg(RMI_DEBUG_CORE, &fn->dev, "Unregistering F%02X.\n", > fn->fd.function_number); > > - device_del(&fn->dev); > - of_node_put(fn->dev.of_node); > - > for (i = 0; i < fn->num_of_irqs; i++) > irq_dispose_mapping(fn->irq[i]); > > + device_del(&fn->dev); > + of_node_put(fn->dev.of_node); > put_device(&fn->dev); > } > Acked-by: Mathias Krause <minipli@grsecurity.net>
On 22.02.24 15:08, Mathias Krause wrote: >> >> The bug is that we must not call device_del() until after calling >> irq_dispose_mapping(). > > Unfortunately, this is only half the truth. We investigated this further > and there's another bug that got introduced in commit 24d28e4f1271 > ("Input: synaptics-rmi4 - convert irq distribution to irq_domain"). The > IRQ domain has a UAF issue as well. I'll send the patch soon -- wanted > to do so this week, but, again, more urgent matters interrupted this. Unfortunately, I send that Email too fast. Looking at the backtrace again, it's just the other bug that needs fixing (a UAF in rmi_driver_remove()). Will sent a patch really soon now! > >> >> Fixes: 24d28e4f1271 ("Input: synaptics-rmi4 - convert irq distribution to irq_domain") >> Reported-by: Torsten Hilbrich <torsten.hilbrich@secunet.com> >> Closes: https://lore.kernel.org/all/1932038e-2776-04ac-5fcd-b15bb3cd088d@secunet.com/ >> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=215604 >> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> >> --- >> drivers/input/rmi4/rmi_bus.c | 5 ++--- >> 1 file changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c >> index 1b45b1d3077d..02acc81b9d3e 100644 >> --- a/drivers/input/rmi4/rmi_bus.c >> +++ b/drivers/input/rmi4/rmi_bus.c >> @@ -275,12 +275,11 @@ void rmi_unregister_function(struct rmi_function *fn) >> rmi_dbg(RMI_DEBUG_CORE, &fn->dev, "Unregistering F%02X.\n", >> fn->fd.function_number); >> >> - device_del(&fn->dev); >> - of_node_put(fn->dev.of_node); >> - >> for (i = 0; i < fn->num_of_irqs; i++) >> irq_dispose_mapping(fn->irq[i]); >> >> + device_del(&fn->dev); >> + of_node_put(fn->dev.of_node); >> put_device(&fn->dev); >> } >> > > Acked-by: Mathias Krause <minipli@grsecurity.net> So, forget about that one, sorry.
On Thu, Feb 22, 2024 at 03:19:49PM +0100, Mathias Krause wrote: > On 22.02.24 15:08, Mathias Krause wrote: > >> > >> The bug is that we must not call device_del() until after calling > >> irq_dispose_mapping(). > > > > Unfortunately, this is only half the truth. We investigated this further > > and there's another bug that got introduced in commit 24d28e4f1271 > > ("Input: synaptics-rmi4 - convert irq distribution to irq_domain"). The > > IRQ domain has a UAF issue as well. I'll send the patch soon -- wanted > > to do so this week, but, again, more urgent matters interrupted this. > > Unfortunately, I send that Email too fast. Looking at the backtrace > again, it's just the other bug that needs fixing (a UAF in > rmi_driver_remove()). Will sent a patch really soon now! > > > > >> > >> Fixes: 24d28e4f1271 ("Input: synaptics-rmi4 - convert irq distribution to irq_domain") > >> Reported-by: Torsten Hilbrich <torsten.hilbrich@secunet.com> > >> Closes: https://lore.kernel.org/all/1932038e-2776-04ac-5fcd-b15bb3cd088d@secunet.com/ > >> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=215604 > >> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > >> --- > >> drivers/input/rmi4/rmi_bus.c | 5 ++--- > >> 1 file changed, 2 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c > >> index 1b45b1d3077d..02acc81b9d3e 100644 > >> --- a/drivers/input/rmi4/rmi_bus.c > >> +++ b/drivers/input/rmi4/rmi_bus.c > >> @@ -275,12 +275,11 @@ void rmi_unregister_function(struct rmi_function *fn) > >> rmi_dbg(RMI_DEBUG_CORE, &fn->dev, "Unregistering F%02X.\n", > >> fn->fd.function_number); > >> > >> - device_del(&fn->dev); > >> - of_node_put(fn->dev.of_node); > >> - > >> for (i = 0; i < fn->num_of_irqs; i++) > >> irq_dispose_mapping(fn->irq[i]); > >> > >> + device_del(&fn->dev); > >> + of_node_put(fn->dev.of_node); > >> put_device(&fn->dev); > >> } > >> > > > > Acked-by: Mathias Krause <minipli@grsecurity.net> > > So, forget about that one, sorry. Right, I do not think this patch is needed. The "fn" will stay in memory until the last reference to fn->dev is dropped. So the original patch was enough for this particular issue, and Mathias' patch fixes slightly different UAF. Thanks.
diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c index 1b45b1d3077d..02acc81b9d3e 100644 --- a/drivers/input/rmi4/rmi_bus.c +++ b/drivers/input/rmi4/rmi_bus.c @@ -275,12 +275,11 @@ void rmi_unregister_function(struct rmi_function *fn) rmi_dbg(RMI_DEBUG_CORE, &fn->dev, "Unregistering F%02X.\n", fn->fd.function_number); - device_del(&fn->dev); - of_node_put(fn->dev.of_node); - for (i = 0; i < fn->num_of_irqs; i++) irq_dispose_mapping(fn->irq[i]); + device_del(&fn->dev); + of_node_put(fn->dev.of_node); put_device(&fn->dev); }