| Message ID | 20200427145537.1.Ic8f898e0147beeee2c005ee7b20f1aebdef1e7eb@changeid (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | d5a5e5b5fa7b86c05bf073acc0ba98fa280174ec |
| Headers | show |
| Series | Input: synaptics-rmi4 - Really fix attn_data use-after-free | expand |
On Mon, Apr 27, 2020 at 02:55:48PM -0700, Evan Green wrote: > Fix a use-after-free noticed by running with KASAN enabled. If > rmi_irq_fn() is run twice in a row, then rmi_f11_attention() (among > others) will end up reading from drvdata->attn_data.data, which was > freed and left dangling in rmi_irq_fn(). > > Commit 55edde9fff1a ("Input: synaptics-rmi4 - prevent UAF reported by > KASAN") correctly identified and analyzed this bug. However the attempted > fix only NULLed out a local variable, missing the fact that > drvdata->attn_data is a struct, not a pointer. > > NULL out the correct pointer in the driver data to prevent the attention > functions from copying from it. > > Fixes: 55edde9fff1a ("Input: synaptics-rmi4 - prevent UAF reported by KASAN") > Fixes: b908d3cd812a ("Input: synaptics-rmi4 - allow to add attention data") > > Signed-off-by: Evan Green <evgreen@chromium.org> > > Cc: stable@vger.kernel.org > Cc: Nick Desaulniers <nick.desaulniers@gmail.com> Ugh, this is all kind of ugly, but I guess that's what we have now. Applied, thank you. > --- > > drivers/input/rmi4/rmi_driver.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c > index 190b9974526bb..c18e1a25bca6e 100644 > --- a/drivers/input/rmi4/rmi_driver.c > +++ b/drivers/input/rmi4/rmi_driver.c > @@ -205,7 +205,7 @@ static irqreturn_t rmi_irq_fn(int irq, void *dev_id) > > if (count) { > kfree(attn_data.data); > - attn_data.data = NULL; > + drvdata->attn_data.data = NULL; > } > > if (!kfifo_is_empty(&drvdata->attn_fifo)) > -- > 2.24.1 >
On Mon, Apr 27, 2020 at 6:11 PM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > On Mon, Apr 27, 2020 at 02:55:48PM -0700, Evan Green wrote: > > Fix a use-after-free noticed by running with KASAN enabled. If > > rmi_irq_fn() is run twice in a row, then rmi_f11_attention() (among > > others) will end up reading from drvdata->attn_data.data, which was > > freed and left dangling in rmi_irq_fn(). > > > > Commit 55edde9fff1a ("Input: synaptics-rmi4 - prevent UAF reported by > > KASAN") correctly identified and analyzed this bug. However the attempted > > fix only NULLed out a local variable, missing the fact that > > drvdata->attn_data is a struct, not a pointer. > > > > NULL out the correct pointer in the driver data to prevent the attention > > functions from copying from it. > > > > Fixes: 55edde9fff1a ("Input: synaptics-rmi4 - prevent UAF reported by KASAN") > > Fixes: b908d3cd812a ("Input: synaptics-rmi4 - allow to add attention data") > > > > Signed-off-by: Evan Green <evgreen@chromium.org> > > > > Cc: stable@vger.kernel.org > > Cc: Nick Desaulniers <nick.desaulniers@gmail.com> > > Ugh, this is all kind of ugly, but I guess that's what we have now. > Applied, thank you. Thanks Dmitry. There are other bits that sketch me out in here as well, but I didn't get a chance to really figure out if they're a problem. We call rmi_process_interrupt_requests(), which results in reads from that same attn_data.data pointer, in a few different places. Some of those calls are outside the irq handling path, like the in rmi_enable_irq() and rmi_enable_sensor(). Can they race with the irq handling path? (Meaning they'd be doing those attn_data.data reads as rmi_irq_fn() is kfreeing the data?) There are a smattering of mutexes around, but I'm not sure if they're trying to protect this. If I can find some time I'll try to submit a patch. Anyone is welcome to beat me to it though. -Evan
diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c index 190b9974526bb..c18e1a25bca6e 100644 --- a/drivers/input/rmi4/rmi_driver.c +++ b/drivers/input/rmi4/rmi_driver.c @@ -205,7 +205,7 @@ static irqreturn_t rmi_irq_fn(int irq, void *dev_id) if (count) { kfree(attn_data.data); - attn_data.data = NULL; + drvdata->attn_data.data = NULL; } if (!kfifo_is_empty(&drvdata->attn_fifo))
Fix a use-after-free noticed by running with KASAN enabled. If rmi_irq_fn() is run twice in a row, then rmi_f11_attention() (among others) will end up reading from drvdata->attn_data.data, which was freed and left dangling in rmi_irq_fn(). Commit 55edde9fff1a ("Input: synaptics-rmi4 - prevent UAF reported by KASAN") correctly identified and analyzed this bug. However the attempted fix only NULLed out a local variable, missing the fact that drvdata->attn_data is a struct, not a pointer. NULL out the correct pointer in the driver data to prevent the attention functions from copying from it. Fixes: 55edde9fff1a ("Input: synaptics-rmi4 - prevent UAF reported by KASAN") Fixes: b908d3cd812a ("Input: synaptics-rmi4 - allow to add attention data") Signed-off-by: Evan Green <evgreen@chromium.org> Cc: stable@vger.kernel.org Cc: Nick Desaulniers <nick.desaulniers@gmail.com> --- drivers/input/rmi4/rmi_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)