* [PATCH resend 1/3] Input: synaptics-rmi4 - fix video buffer size
@ 2019-11-04 11:44 Lucas Stach
2019-11-04 11:44 ` [PATCH resend 2/3] Input: synaptics-rmi4 - add dummy F54 attention handler Lucas Stach
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Lucas Stach @ 2019-11-04 11:44 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, patchwork-lst, kernel
The video buffer used by the queue is a vb2_v4l2_buffer, not a plain
vb2_buffer. Using the wrong type causes the allocation of the buffer
storage to be too small, causing a out of bounds write when
__init_vb2_v4l2_buffer initializes the buffer.
Fixes: 3a762dbd5347 ("[media] Input: synaptics-rmi4 - add support
for F54 diagnostics")
Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
drivers/input/rmi4/rmi_f54.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/rmi4/rmi_f54.c b/drivers/input/rmi4/rmi_f54.c
index 710b02595486..4841354af0d7 100644
--- a/drivers/input/rmi4/rmi_f54.c
+++ b/drivers/input/rmi4/rmi_f54.c
@@ -359,7 +359,7 @@ static const struct vb2_ops rmi_f54_queue_ops = {
static const struct vb2_queue rmi_f54_queue = {
.type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
.io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF | VB2_READ,
- .buf_struct_size = sizeof(struct vb2_buffer),
+ .buf_struct_size = sizeof(struct vb2_v4l2_buffer),
.ops = &rmi_f54_queue_ops,
.mem_ops = &vb2_vmalloc_memops,
.timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC,
--
2.20.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH resend 2/3] Input: synaptics-rmi4 - add dummy F54 attention handler 2019-11-04 11:44 [PATCH resend 1/3] Input: synaptics-rmi4 - fix video buffer size Lucas Stach @ 2019-11-04 11:44 ` Lucas Stach 2019-11-05 0:03 ` Dmitry Torokhov 2019-11-04 11:44 ` [PATCH resend 3/3] Input: synaptics-rmi4 - simplify data read in rmi_f54_work Lucas Stach 2019-11-05 0:02 ` [PATCH resend 1/3] Input: synaptics-rmi4 - fix video buffer size Dmitry Torokhov 2 siblings, 1 reply; 7+ messages in thread From: Lucas Stach @ 2019-11-04 11:44 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: linux-input, patchwork-lst, kernel F54 is IRQ capable, even if it is not used in the current driver implementation. The common driver code in rmi_create_function_irq always installs a irq handler for functions that are IRQ capable. Without a assigned attention handler, this means a NULL pointer being passed as the nested IRQ handler. This seems to work with some architecture implementations, but crashes on others like ARM64. Don't rely on implementation defined behavior and actually install a proper attention handler. Fixes: 24d28e4f1271 ("Input: synaptics-rmi4 - convert irq distribution to irq_domain") Signed-off-by: Lucas Stach <l.stach@pengutronix.de> --- drivers/input/rmi4/rmi_f54.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/input/rmi4/rmi_f54.c b/drivers/input/rmi4/rmi_f54.c index 4841354af0d7..22390e89c680 100644 --- a/drivers/input/rmi4/rmi_f54.c +++ b/drivers/input/rmi4/rmi_f54.c @@ -732,6 +732,11 @@ static void rmi_f54_remove(struct rmi_function *fn) v4l2_device_unregister(&f54->v4l2); } +static irqreturn_t rmi_f54_attention(int irq, void *ctx) +{ + return IRQ_HANDLED; +} + struct rmi_function_handler rmi_f54_handler = { .driver = { .name = F54_NAME, @@ -740,4 +745,5 @@ struct rmi_function_handler rmi_f54_handler = { .probe = rmi_f54_probe, .config = rmi_f54_config, .remove = rmi_f54_remove, + .attention = rmi_f54_attention, }; -- 2.20.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH resend 2/3] Input: synaptics-rmi4 - add dummy F54 attention handler 2019-11-04 11:44 ` [PATCH resend 2/3] Input: synaptics-rmi4 - add dummy F54 attention handler Lucas Stach @ 2019-11-05 0:03 ` Dmitry Torokhov 2019-11-05 11:46 ` Lucas Stach 0 siblings, 1 reply; 7+ messages in thread From: Dmitry Torokhov @ 2019-11-05 0:03 UTC (permalink / raw) To: Lucas Stach; +Cc: linux-input, patchwork-lst, kernel On Mon, Nov 04, 2019 at 12:44:53PM +0100, Lucas Stach wrote: > F54 is IRQ capable, even if it is not used in the current driver > implementation. The common driver code in rmi_create_function_irq always > installs a irq handler for functions that are IRQ capable. Without a > assigned attention handler, this means a NULL pointer being passed as > the nested IRQ handler. This seems to work with some architecture > implementations, but crashes on others like ARM64. > > Don't rely on implementation defined behavior and actually install > a proper attention handler. Instead of supplying dummy IRQ handler, can't we simply disable relevant interrupts bits? > > Fixes: 24d28e4f1271 ("Input: synaptics-rmi4 - convert irq distribution > to irq_domain") > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > --- > drivers/input/rmi4/rmi_f54.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/input/rmi4/rmi_f54.c b/drivers/input/rmi4/rmi_f54.c > index 4841354af0d7..22390e89c680 100644 > --- a/drivers/input/rmi4/rmi_f54.c > +++ b/drivers/input/rmi4/rmi_f54.c > @@ -732,6 +732,11 @@ static void rmi_f54_remove(struct rmi_function *fn) > v4l2_device_unregister(&f54->v4l2); > } > > +static irqreturn_t rmi_f54_attention(int irq, void *ctx) > +{ > + return IRQ_HANDLED; > +} > + > struct rmi_function_handler rmi_f54_handler = { > .driver = { > .name = F54_NAME, > @@ -740,4 +745,5 @@ struct rmi_function_handler rmi_f54_handler = { > .probe = rmi_f54_probe, > .config = rmi_f54_config, > .remove = rmi_f54_remove, > + .attention = rmi_f54_attention, > }; > -- > 2.20.1 > Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH resend 2/3] Input: synaptics-rmi4 - add dummy F54 attention handler 2019-11-05 0:03 ` Dmitry Torokhov @ 2019-11-05 11:46 ` Lucas Stach 0 siblings, 0 replies; 7+ messages in thread From: Lucas Stach @ 2019-11-05 11:46 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: linux-input, patchwork-lst, kernel Hi Dmitry, On Mo, 2019-11-04 at 16:03 -0800, Dmitry Torokhov wrote: > On Mon, Nov 04, 2019 at 12:44:53PM +0100, Lucas Stach wrote: > > F54 is IRQ capable, even if it is not used in the current driver > > implementation. The common driver code in rmi_create_function_irq always > > installs a irq handler for functions that are IRQ capable. Without a > > assigned attention handler, this means a NULL pointer being passed as > > the nested IRQ handler. This seems to work with some architecture > > implementations, but crashes on others like ARM64. > > > > Don't rely on implementation defined behavior and actually install > > a proper attention handler. > > Instead of supplying dummy IRQ handler, can't we simply disable relevant > interrupts bits? Don't know why I didn't try this last time. I vaguely remember that not enabling the IRQs didn't help. I just retested and it seems the IRQs are enabled by default, so we need to actively disable them. I just sent out a patch which does exactly this. Regards, Lucas ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH resend 3/3] Input: synaptics-rmi4 - simplify data read in rmi_f54_work 2019-11-04 11:44 [PATCH resend 1/3] Input: synaptics-rmi4 - fix video buffer size Lucas Stach 2019-11-04 11:44 ` [PATCH resend 2/3] Input: synaptics-rmi4 - add dummy F54 attention handler Lucas Stach @ 2019-11-04 11:44 ` Lucas Stach 2019-11-05 0:02 ` Dmitry Torokhov 2019-11-05 0:02 ` [PATCH resend 1/3] Input: synaptics-rmi4 - fix video buffer size Dmitry Torokhov 2 siblings, 1 reply; 7+ messages in thread From: Lucas Stach @ 2019-11-04 11:44 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: linux-input, patchwork-lst, kernel The body of the for loop is only ever run once as the second standard_report element is never changed from its initial zero init, so the loop condition is never satisfies after the first run. Equally the start member of the first element is never changed from 0, so the index offset is always a constant 0. Remove this needless obfuscation of the code and write it in a straight forward manner. Signed-off-by: Lucas Stach <l.stach@pengutronix.de> --- drivers/input/rmi4/rmi_f54.c | 48 ++++++++++++------------------------ 1 file changed, 16 insertions(+), 32 deletions(-) diff --git a/drivers/input/rmi4/rmi_f54.c b/drivers/input/rmi4/rmi_f54.c index 22390e89c680..5b1799bdfb62 100644 --- a/drivers/input/rmi4/rmi_f54.c +++ b/drivers/input/rmi4/rmi_f54.c @@ -81,11 +81,6 @@ static const char * const rmi_f54_report_type_names[] = { = "Full Raw Capacitance RX Offset Removed", }; -struct rmi_f54_reports { - int start; - int size; -}; - struct f54_data { struct rmi_function *fn; @@ -98,7 +93,6 @@ struct f54_data { enum rmi_f54_report_type report_type; u8 *report_data; int report_size; - struct rmi_f54_reports standard_report[2]; bool is_busy; struct mutex status_mutex; @@ -516,13 +510,10 @@ static void rmi_f54_work(struct work_struct *work) struct f54_data *f54 = container_of(work, struct f54_data, work.work); struct rmi_function *fn = f54->fn; u8 fifo[2]; - struct rmi_f54_reports *report; int report_size; u8 command; - u8 *data; int error; - data = f54->report_data; report_size = rmi_f54_get_report_size(f54); if (report_size == 0) { dev_err(&fn->dev, "Bad report size, report type=%d\n", @@ -530,8 +521,6 @@ static void rmi_f54_work(struct work_struct *work) error = -EINVAL; goto error; /* retry won't help */ } - f54->standard_report[0].size = report_size; - report = f54->standard_report; mutex_lock(&f54->data_mutex); @@ -556,28 +545,23 @@ static void rmi_f54_work(struct work_struct *work) rmi_dbg(RMI_DEBUG_FN, &fn->dev, "Get report command completed, reading data\n"); - report_size = 0; - for (; report->size; report++) { - fifo[0] = report->start & 0xff; - fifo[1] = (report->start >> 8) & 0xff; - error = rmi_write_block(fn->rmi_dev, - fn->fd.data_base_addr + F54_FIFO_OFFSET, - fifo, sizeof(fifo)); - if (error) { - dev_err(&fn->dev, "Failed to set fifo start offset\n"); - goto abort; - } + fifo[0] = 0; + fifo[1] = 0; + error = rmi_write_block(fn->rmi_dev, + fn->fd.data_base_addr + F54_FIFO_OFFSET, + fifo, sizeof(fifo)); + if (error) { + dev_err(&fn->dev, "Failed to set fifo start offset\n"); + goto abort; + } - error = rmi_read_block(fn->rmi_dev, fn->fd.data_base_addr + - F54_REPORT_DATA_OFFSET, data, - report->size); - if (error) { - dev_err(&fn->dev, "%s: read [%d bytes] returned %d\n", - __func__, report->size, error); - goto abort; - } - data += report->size; - report_size += report->size; + error = rmi_read_block(fn->rmi_dev, fn->fd.data_base_addr + + F54_REPORT_DATA_OFFSET, f54->report_data, + report_size); + if (error) { + dev_err(&fn->dev, "%s: read [%d bytes] returned %d\n", + __func__, report_size, error); + goto abort; } abort: -- 2.20.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH resend 3/3] Input: synaptics-rmi4 - simplify data read in rmi_f54_work 2019-11-04 11:44 ` [PATCH resend 3/3] Input: synaptics-rmi4 - simplify data read in rmi_f54_work Lucas Stach @ 2019-11-05 0:02 ` Dmitry Torokhov 0 siblings, 0 replies; 7+ messages in thread From: Dmitry Torokhov @ 2019-11-05 0:02 UTC (permalink / raw) To: Lucas Stach; +Cc: linux-input, patchwork-lst, kernel On Mon, Nov 04, 2019 at 12:44:54PM +0100, Lucas Stach wrote: > The body of the for loop is only ever run once as the second standard_report > element is never changed from its initial zero init, so the loop condition is > never satisfies after the first run. Equally the start member of the first > element is never changed from 0, so the index offset is always a constant 0. > > Remove this needless obfuscation of the code and write it in a straight > forward manner. > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> Applied, thank you. > --- > drivers/input/rmi4/rmi_f54.c | 48 ++++++++++++------------------------ > 1 file changed, 16 insertions(+), 32 deletions(-) > > diff --git a/drivers/input/rmi4/rmi_f54.c b/drivers/input/rmi4/rmi_f54.c > index 22390e89c680..5b1799bdfb62 100644 > --- a/drivers/input/rmi4/rmi_f54.c > +++ b/drivers/input/rmi4/rmi_f54.c > @@ -81,11 +81,6 @@ static const char * const rmi_f54_report_type_names[] = { > = "Full Raw Capacitance RX Offset Removed", > }; > > -struct rmi_f54_reports { > - int start; > - int size; > -}; > - > struct f54_data { > struct rmi_function *fn; > > @@ -98,7 +93,6 @@ struct f54_data { > enum rmi_f54_report_type report_type; > u8 *report_data; > int report_size; > - struct rmi_f54_reports standard_report[2]; > > bool is_busy; > struct mutex status_mutex; > @@ -516,13 +510,10 @@ static void rmi_f54_work(struct work_struct *work) > struct f54_data *f54 = container_of(work, struct f54_data, work.work); > struct rmi_function *fn = f54->fn; > u8 fifo[2]; > - struct rmi_f54_reports *report; > int report_size; > u8 command; > - u8 *data; > int error; > > - data = f54->report_data; > report_size = rmi_f54_get_report_size(f54); > if (report_size == 0) { > dev_err(&fn->dev, "Bad report size, report type=%d\n", > @@ -530,8 +521,6 @@ static void rmi_f54_work(struct work_struct *work) > error = -EINVAL; > goto error; /* retry won't help */ > } > - f54->standard_report[0].size = report_size; > - report = f54->standard_report; > > mutex_lock(&f54->data_mutex); > > @@ -556,28 +545,23 @@ static void rmi_f54_work(struct work_struct *work) > > rmi_dbg(RMI_DEBUG_FN, &fn->dev, "Get report command completed, reading data\n"); > > - report_size = 0; > - for (; report->size; report++) { > - fifo[0] = report->start & 0xff; > - fifo[1] = (report->start >> 8) & 0xff; > - error = rmi_write_block(fn->rmi_dev, > - fn->fd.data_base_addr + F54_FIFO_OFFSET, > - fifo, sizeof(fifo)); > - if (error) { > - dev_err(&fn->dev, "Failed to set fifo start offset\n"); > - goto abort; > - } > + fifo[0] = 0; > + fifo[1] = 0; > + error = rmi_write_block(fn->rmi_dev, > + fn->fd.data_base_addr + F54_FIFO_OFFSET, > + fifo, sizeof(fifo)); > + if (error) { > + dev_err(&fn->dev, "Failed to set fifo start offset\n"); > + goto abort; > + } > > - error = rmi_read_block(fn->rmi_dev, fn->fd.data_base_addr + > - F54_REPORT_DATA_OFFSET, data, > - report->size); > - if (error) { > - dev_err(&fn->dev, "%s: read [%d bytes] returned %d\n", > - __func__, report->size, error); > - goto abort; > - } > - data += report->size; > - report_size += report->size; > + error = rmi_read_block(fn->rmi_dev, fn->fd.data_base_addr + > + F54_REPORT_DATA_OFFSET, f54->report_data, > + report_size); > + if (error) { > + dev_err(&fn->dev, "%s: read [%d bytes] returned %d\n", > + __func__, report_size, error); > + goto abort; > } > > abort: > -- > 2.20.1 > -- Dmitry ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH resend 1/3] Input: synaptics-rmi4 - fix video buffer size 2019-11-04 11:44 [PATCH resend 1/3] Input: synaptics-rmi4 - fix video buffer size Lucas Stach 2019-11-04 11:44 ` [PATCH resend 2/3] Input: synaptics-rmi4 - add dummy F54 attention handler Lucas Stach 2019-11-04 11:44 ` [PATCH resend 3/3] Input: synaptics-rmi4 - simplify data read in rmi_f54_work Lucas Stach @ 2019-11-05 0:02 ` Dmitry Torokhov 2 siblings, 0 replies; 7+ messages in thread From: Dmitry Torokhov @ 2019-11-05 0:02 UTC (permalink / raw) To: Lucas Stach; +Cc: linux-input, patchwork-lst, kernel On Mon, Nov 04, 2019 at 12:44:52PM +0100, Lucas Stach wrote: > The video buffer used by the queue is a vb2_v4l2_buffer, not a plain > vb2_buffer. Using the wrong type causes the allocation of the buffer > storage to be too small, causing a out of bounds write when > __init_vb2_v4l2_buffer initializes the buffer. > > Fixes: 3a762dbd5347 ("[media] Input: synaptics-rmi4 - add support > for F54 diagnostics") > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> Applied, thank you. > --- > drivers/input/rmi4/rmi_f54.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/input/rmi4/rmi_f54.c b/drivers/input/rmi4/rmi_f54.c > index 710b02595486..4841354af0d7 100644 > --- a/drivers/input/rmi4/rmi_f54.c > +++ b/drivers/input/rmi4/rmi_f54.c > @@ -359,7 +359,7 @@ static const struct vb2_ops rmi_f54_queue_ops = { > static const struct vb2_queue rmi_f54_queue = { > .type = V4L2_BUF_TYPE_VIDEO_CAPTURE, > .io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF | VB2_READ, > - .buf_struct_size = sizeof(struct vb2_buffer), > + .buf_struct_size = sizeof(struct vb2_v4l2_buffer), > .ops = &rmi_f54_queue_ops, > .mem_ops = &vb2_vmalloc_memops, > .timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC, > -- > 2.20.1 > -- Dmitry ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-11-05 11:46 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-11-04 11:44 [PATCH resend 1/3] Input: synaptics-rmi4 - fix video buffer size Lucas Stach 2019-11-04 11:44 ` [PATCH resend 2/3] Input: synaptics-rmi4 - add dummy F54 attention handler Lucas Stach 2019-11-05 0:03 ` Dmitry Torokhov 2019-11-05 11:46 ` Lucas Stach 2019-11-04 11:44 ` [PATCH resend 3/3] Input: synaptics-rmi4 - simplify data read in rmi_f54_work Lucas Stach 2019-11-05 0:02 ` Dmitry Torokhov 2019-11-05 0:02 ` [PATCH resend 1/3] Input: synaptics-rmi4 - fix video buffer size Dmitry Torokhov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).