'struct hidraw_list' is a circular queue whose head can be smaller than
tail. Using 'list->tail != list->head' to release all memory that should
be released.
Fixes: a5623a203cff ("HID: hidraw: fix memory leak in hidraw_release()")
Signed-off-by: Su Hui <[email protected]>
---
drivers/hid/hidraw.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
index 13c8dd8cd350..2bc762d31ac7 100644
--- a/drivers/hid/hidraw.c
+++ b/drivers/hid/hidraw.c
@@ -357,8 +357,11 @@ static int hidraw_release(struct inode * inode, struct file * file)
down_write(&minors_rwsem);
spin_lock_irqsave(&hidraw_table[minor]->list_lock, flags);
- for (int i = list->tail; i < list->head; i++)
- kfree(list->buffer[i].value);
+ while (list->tail != list->head) {
+ kfree(list->buffer[list->tail].value);
+ list->buffer[list->tail].value = NULL;
+ list->tail = (list->tail + 1) & (HIDRAW_BUFFER_SIZE - 1);
+ }
list_del(&list->node);
spin_unlock_irqrestore(&hidraw_table[minor]->list_lock, flags);
kfree(list);
--
2.30.2
On Thu, Jan 25, 2024 at 02:32:26PM +0800, Su Hui wrote:
> 'struct hidraw_list' is a circular queue whose head can be smaller than
> tail. Using 'list->tail != list->head' to release all memory that should
> be released.
>
> Fixes: a5623a203cff ("HID: hidraw: fix memory leak in hidraw_release()")
> Signed-off-by: Su Hui <[email protected]>
This is very clever. How did you find that? Was it through static
analysis or just review? Perhaps using syzkaller?
Reviewed-by: Dan Carpenter <[email protected]>
I imagine we could write a checker heuristic to identify ->tail and
->head struct members and then complain if they were ever used in a <
or > comparison.
regards,
dan carpenter
On 2024/1/25 15:11, Dan Carpenter wrote:
> On Thu, Jan 25, 2024 at 02:32:26PM +0800, Su Hui wrote:
>> 'struct hidraw_list' is a circular queue whose head can be smaller than
>> tail. Using 'list->tail != list->head' to release all memory that should
>> be released.
>>
>> Fixes: a5623a203cff ("HID: hidraw: fix memory leak in hidraw_release()")
>> Signed-off-by: Su Hui <[email protected]>
> This is very clever. How did you find that? Was it through static
> analysis or just review? Perhaps using syzkaller?
Hi,
I just met this bug on a real machine and found this problem by
reviewing the code.
>
> Reviewed-by: Dan Carpenter <[email protected]>
>
> I imagine we could write a checker heuristic to identify ->tail and
> ->head struct members and then complain if they were ever used in a <
> or > comparison.
I can't agree any more, great idea!
Su Hui
On Thu, 25 Jan 2024, Su Hui wrote:
> 'struct hidraw_list' is a circular queue whose head can be smaller than
> tail. Using 'list->tail != list->head' to release all memory that should
> be released.
>
> Fixes: a5623a203cff ("HID: hidraw: fix memory leak in hidraw_release()")
> Signed-off-by: Su Hui <[email protected]>
Good catch, thanks. Applied.
--
Jiri Kosina
SUSE Labs