[PATCH 0/3] HID: Fix some memory leaks in drivers/hid

[PATCH 0/3] HID: Fix some memory leaks in drivers/hid

[Günther Noack]

Hello!

These patches fix a few memory leaks in HID report descriptor fixups.

FWIW, a good ad-hoc way to look for usages of allocation functions in
these is:

  awk '/static.*report_fixup.*/,/^}/ { print FILENAME, $0 }' drivers/hid/hid-*.c \
    | grep -E '(malloc|kzalloc|kcalloc|kmemdup)'

The devm_* variants are safe in this context, because they tie the
allocated memory to the lifetime of the driver.

For transparency, I generated these commits with Gemini-CLI,
starting with this prompt:

    We are working in the Linux kernel. In the HID drivers in
    `drivers/hid/hid-*.c`, the `report_fixup` driver hook is a function
    that gets a byte buffer (with size) as input and that may modify that
    byte buffer, and optionally return a pointer to a new byte buffer and
    update the size.  The returned value is *not* memory-managed by the
    caller though and will not be freed subsequently.  When the
    `report_fixup` implementation allocates a new buffer and returns that,
    that will not get freed by the caller.  Validate this assessment and
    fix up all HID drivers where that mistake is made.

(and then a little bit of additional nudging for the details).

—Günther


Günther Noack (3):
  HID: apple: avoid memory leak in apple_report_fixup()
  HID: magicmouse: avoid memory leak in magicmouse_report_fixup()
  HID: asus: avoid memory leak in asus_report_fixup()

 drivers/hid/hid-apple.c      |  4 +---
 drivers/hid/hid-asus.c       | 15 +++++++++++----
 drivers/hid/hid-magicmouse.c |  4 +---
 3 files changed, 13 insertions(+), 10 deletions(-)

-- 
2.53.0.335.g19a08e0c02-goog

[PATCH 1/3] HID: apple: avoid memory leak in apple_report_fixup()

[Günther Noack]

The apple_report_fixup() function was allocating a new buffer with
kmemdup() but never freeing it. Since the caller of report_fixup() already
provides a writable buffer and allows returning a pointer within that
buffer, we can just modify the descriptor in-place and return the adjusted
pointer.

Assisted-by: Gemini-CLI:Google Gemini 3
Signed-off-by: Günther Noack <gnoack@google.com>
---
 drivers/hid/hid-apple.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
index 233e367cce1d..894adc23367b 100644
--- a/drivers/hid/hid-apple.c
+++ b/drivers/hid/hid-apple.c
@@ -686,9 +686,7 @@ static const __u8 *apple_report_fixup(struct hid_device *hdev, __u8 *rdesc,
 		hid_info(hdev,
 			 "fixing up Magic Keyboard battery report descriptor\n");
 		*rsize = *rsize - 1;
-		rdesc = kmemdup(rdesc + 1, *rsize, GFP_KERNEL);
-		if (!rdesc)
-			return NULL;
+		rdesc = rdesc + 1;
 
 		rdesc[0] = 0x05;
 		rdesc[1] = 0x01;
-- 
2.53.0.335.g19a08e0c02-goog

[PATCH 2/3] HID: magicmouse: avoid memory leak in magicmouse_report_fixup()

[Günther Noack]

The magicmouse_report_fixup() function was allocating a new buffer with
kmemdup() but never freeing it. Since the caller already provides a
writable buffer and allows returning a pointer within it, modify the
descriptor in-place and return the adjusted pointer.

Assisted-by: Gemini-CLI:Google Gemini 3
Signed-off-by: Günther Noack <gnoack@google.com>
---
 drivers/hid/hid-magicmouse.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 91f621ceb924..17908d52c027 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -994,9 +994,7 @@ static const __u8 *magicmouse_report_fixup(struct hid_device *hdev, __u8 *rdesc,
 		hid_info(hdev,
 			 "fixing up magicmouse battery report descriptor\n");
 		*rsize = *rsize - 1;
-		rdesc = kmemdup(rdesc + 1, *rsize, GFP_KERNEL);
-		if (!rdesc)
-			return NULL;
+		rdesc = rdesc + 1;
 
 		rdesc[0] = 0x05;
 		rdesc[1] = 0x01;
-- 
2.53.0.335.g19a08e0c02-goog

[PATCH 3/3] HID: asus: avoid memory leak in asus_report_fixup()

[Günther Noack]

The asus_report_fixup() function was allocating a new buffer with kmemdup()
when growing the report descriptor but never freeing it.  Switch to
devm_kzalloc() to ensure the memory is managed and freed automatically when
the device is removed.

Also fix a harmless out-of-bounds read by copying only the original
descriptor size.

Assisted-by: Gemini-CLI:Google Gemini 3
Signed-off-by: Günther Noack <gnoack@google.com>
---
 drivers/hid/hid-asus.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 8ffcd12038e8..7a08e964b9cc 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -1399,14 +1399,21 @@ static const __u8 *asus_report_fixup(struct hid_device *hdev, __u8 *rdesc,
 		 */
 		if (*rsize == rsize_orig &&
 			rdesc[offs] == 0x09 && rdesc[offs + 1] == 0x76) {
-			*rsize = rsize_orig + 1;
-			rdesc = kmemdup(rdesc, *rsize, GFP_KERNEL);
-			if (!rdesc)
-				return NULL;
+			__u8 *new_rdesc;
+
+			new_rdesc = devm_kzalloc(&hdev->dev, rsize_orig + 1,
+						 GFP_KERNEL);
+			if (!new_rdesc)
+				return rdesc;
 
 			hid_info(hdev, "Fixing up %s keyb report descriptor\n",
 				drvdata->quirks & QUIRK_T100CHI ?
 				"T100CHI" : "T90CHI");
+
+			memcpy(new_rdesc, rdesc, rsize_orig);
+			*rsize = rsize_orig + 1;
+			rdesc = new_rdesc;
+
 			memmove(rdesc + offs + 4, rdesc + offs + 2, 12);
 			rdesc[offs] = 0x19;
 			rdesc[offs + 1] = 0x00;
-- 
2.53.0.335.g19a08e0c02-goog

Re: [PATCH 1/3] HID: apple: avoid memory leak in apple_report_fixup()

[Benjamin Tissoires]

On Feb 17 2026, Günther Noack wrote:
> The apple_report_fixup() function was allocating a new buffer with
> kmemdup() but never freeing it. Since the caller of report_fixup() already
> provides a writable buffer and allows returning a pointer within that
> buffer, we can just modify the descriptor in-place and return the adjusted
> pointer.
> 
> Assisted-by: Gemini-CLI:Google Gemini 3
> Signed-off-by: Günther Noack <gnoack@google.com>
> ---
>  drivers/hid/hid-apple.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
> index 233e367cce1d..894adc23367b 100644
> --- a/drivers/hid/hid-apple.c
> +++ b/drivers/hid/hid-apple.c
> @@ -686,9 +686,7 @@ static const __u8 *apple_report_fixup(struct hid_device *hdev, __u8 *rdesc,
>  		hid_info(hdev,
>  			 "fixing up Magic Keyboard battery report descriptor\n");
>  		*rsize = *rsize - 1;
> -		rdesc = kmemdup(rdesc + 1, *rsize, GFP_KERNEL);
> -		if (!rdesc)
> -			return NULL;
> +		rdesc = rdesc + 1;

I might be wrong, but later we call free(dev->rdesc) on device removal.
AFAICT, incrementing the pointer is undefined behavior.

What we should do instead is probably a krealloc instead of a kmemdup.

Same for all 3 patches.

Cheers,
Benjamin

>  
>  		rdesc[0] = 0x05;
>  		rdesc[1] = 0x01;
> -- 
> 2.53.0.335.g19a08e0c02-goog
> 

Re: [PATCH 3/3] HID: asus: avoid memory leak in asus_report_fixup()

[Benjamin Tissoires]

On Feb 17 2026, Günther Noack wrote:
> The asus_report_fixup() function was allocating a new buffer with kmemdup()
> when growing the report descriptor but never freeing it.  Switch to
> devm_kzalloc() to ensure the memory is managed and freed automatically when
> the device is removed.

Actually this one is even worse: you can't use devm_kzalloc because
hid-core.c will later call kfree(dev->rdesc) if dev->rdesc is different
from the one provided by the low level driver. So we are going to have
a double free.

I really wonder if this was ever tested.

Cheers,
Benjamin

> 
> Also fix a harmless out-of-bounds read by copying only the original
> descriptor size.
> 
> Assisted-by: Gemini-CLI:Google Gemini 3
> Signed-off-by: Günther Noack <gnoack@google.com>
> ---
>  drivers/hid/hid-asus.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 8ffcd12038e8..7a08e964b9cc 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -1399,14 +1399,21 @@ static const __u8 *asus_report_fixup(struct hid_device *hdev, __u8 *rdesc,
>  		 */
>  		if (*rsize == rsize_orig &&
>  			rdesc[offs] == 0x09 && rdesc[offs + 1] == 0x76) {
> -			*rsize = rsize_orig + 1;
> -			rdesc = kmemdup(rdesc, *rsize, GFP_KERNEL);
> -			if (!rdesc)
> -				return NULL;
> +			__u8 *new_rdesc;
> +
> +			new_rdesc = devm_kzalloc(&hdev->dev, rsize_orig + 1,
> +						 GFP_KERNEL);
> +			if (!new_rdesc)
> +				return rdesc;
>  
>  			hid_info(hdev, "Fixing up %s keyb report descriptor\n",
>  				drvdata->quirks & QUIRK_T100CHI ?
>  				"T100CHI" : "T90CHI");
> +
> +			memcpy(new_rdesc, rdesc, rsize_orig);
> +			*rsize = rsize_orig + 1;
> +			rdesc = new_rdesc;
> +
>  			memmove(rdesc + offs + 4, rdesc + offs + 2, 12);
>  			rdesc[offs] = 0x19;
>  			rdesc[offs + 1] = 0x00;
> -- 
> 2.53.0.335.g19a08e0c02-goog
> 

Re: [PATCH 0/3] HID: Fix some memory leaks in drivers/hid

[Benjamin Tissoires]

On Feb 17 2026, Günther Noack wrote:
> Hello!
> 
> These patches fix a few memory leaks in HID report descriptor fixups.
> 
> FWIW, a good ad-hoc way to look for usages of allocation functions in
> these is:
> 
>   awk '/static.*report_fixup.*/,/^}/ { print FILENAME, $0 }' drivers/hid/hid-*.c \
>     | grep -E '(malloc|kzalloc|kcalloc|kmemdup)'
> 
> The devm_* variants are safe in this context, because they tie the
> allocated memory to the lifetime of the driver.

No. Look at hid_close_report() in drivers/hid/hid-core.c.

HID still hasn't fully migrated to devm, so as a rule of thumb, if you
change a kzalloc into a devm_kzalloc, you are getting into troubles
unless you fix the all the kfree path.

> 
> For transparency, I generated these commits with Gemini-CLI,
> starting with this prompt:
> 
>     We are working in the Linux kernel. In the HID drivers in
>     `drivers/hid/hid-*.c`, the `report_fixup` driver hook is a function
>     that gets a byte buffer (with size) as input and that may modify that
>     byte buffer, and optionally return a pointer to a new byte buffer and
>     update the size.  The returned value is *not* memory-managed by the
>     caller though and will not be freed subsequently.  When the

If the memory is *not* managed, why would gemini converts kzalloc into
devm variants without changing the kfree paths????

>     `report_fixup` implementation allocates a new buffer and returns that,
>     that will not get freed by the caller.  

This is wrong. See hid_close_report(): if the new rdesc (after fixup)
differs from the one initially set, there is an explicit call to
kfree().

-> there is no memleak AFAICT, and your prompt is wrong.

Cheers,
Benjamin

> Validate this assessment and
>     fix up all HID drivers where that mistake is made.
> 
> (and then a little bit of additional nudging for the details).
> 
> —Günther
> 
> 
> Günther Noack (3):
>   HID: apple: avoid memory leak in apple_report_fixup()
>   HID: magicmouse: avoid memory leak in magicmouse_report_fixup()
>   HID: asus: avoid memory leak in asus_report_fixup()
> 
>  drivers/hid/hid-apple.c      |  4 +---
>  drivers/hid/hid-asus.c       | 15 +++++++++++----
>  drivers/hid/hid-magicmouse.c |  4 +---
>  3 files changed, 13 insertions(+), 10 deletions(-)
> 
> -- 
> 2.53.0.335.g19a08e0c02-goog
> 

Re: [PATCH 1/3] HID: apple: avoid memory leak in apple_report_fixup()

[Günther Noack]

Hello Benjamin!

On Tue, Feb 17, 2026 at 07:22:20PM +0100, Benjamin Tissoires wrote:
> On Feb 17 2026, Günther Noack wrote:
> > The apple_report_fixup() function was allocating a new buffer with
> > kmemdup() but never freeing it. Since the caller of report_fixup() already
> > provides a writable buffer and allows returning a pointer within that
> > buffer, we can just modify the descriptor in-place and return the adjusted
> > pointer.
> > 
> > Assisted-by: Gemini-CLI:Google Gemini 3
> > Signed-off-by: Günther Noack <gnoack@google.com>
> > ---
> >  drivers/hid/hid-apple.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
> > index 233e367cce1d..894adc23367b 100644
> > --- a/drivers/hid/hid-apple.c
> > +++ b/drivers/hid/hid-apple.c
> > @@ -686,9 +686,7 @@ static const __u8 *apple_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> >  		hid_info(hdev,
> >  			 "fixing up Magic Keyboard battery report descriptor\n");
> >  		*rsize = *rsize - 1;
> > -		rdesc = kmemdup(rdesc + 1, *rsize, GFP_KERNEL);
> > -		if (!rdesc)
> > -			return NULL;
> > +		rdesc = rdesc + 1;
> 
> I might be wrong, but later we call free(dev->rdesc) on device removal.
> AFAICT, incrementing the pointer is undefined behavior.
> 
> What we should do instead is probably a krealloc instead of a kmemdup.
> 
> Same for all 3 patches.

Thanks for the review.

Let me try to address your three responses in one reply.

I am happy to accept it if I am wrong about this, but I don't
understand your reasoning.  (This should go without saying, but maybe
is worth reiterating, I would not have sent this if I had not
convinced myself independently of LLM-assisted reasoning.)

Let me explain my reasoning based on the place where .report_fixup()
is called from, which is hid_open_report() in hid-core.c:


	start = device->bpf_rdesc;                         /* (1) */
	if (WARN_ON(!start))
		return -ENODEV;
	size = device->bpf_rsize;

	if (device->driver->report_fixup) {
		/*
		 * device->driver->report_fixup() needs to work
		 * on a copy of our report descriptor so it can
		 * change it.
		 */
		__u8 *buf = kmemdup(start, size, GFP_KERNEL);   /* (2) */

		if (buf == NULL)
			return -ENOMEM;

		start = device->driver->report_fixup(device, buf, &size);  /* (3) */

		/*
		 * The second kmemdup is required in case report_fixup() returns
		 * a static read-only memory, but we have no idea if that memory
		 * needs to be cleaned up or not at the end.
		 */
		start = kmemdup(start, size, GFP_KERNEL);  /* (4) */
		kfree(buf);                                /* (5) */
		if (start == NULL)
			return -ENOMEM;
	}

	device->rdesc = start;
	device->rsize = size;


This function uses a slightly elaborate scheme to call .report_fixup:

(1) start is assigned to the original device->bpf_rdesc
(2) buf is assigned to a copy of the 'start' buffer (deallocated in (5)).
 .  It is done because buf is meant to be mutated by .report_fixup()
 .  (3) start = ...->report_fixup(..., buf, ...)
 .  (4) start = kmemdup(start, ...)
(5) deallocate buf

Importantly:

(a) The buffer buf passed to report_fixup() is a copy of the report
    descriptor whose lifetime spans only from (2) to (5).
(b) The result of .report_fixup(), start, is immediately discarded in
    (4) and reassigned to the start variable again.

From (b), we can see that the result of .report_fixup() does *not* get
deallocated by the caller, and thus, when the driver wants to return a
newly allocated array, is must also hold a reference to it so that it
can deallocate it later.

From (a), we can see that the report_fixup hook is free to manipulate
the contents of the buffer that it receives, but if we were to
*reallocate* it within report_fixup, as you are suggesting above, it
could become a double free:

* During realloc, the allocator would potentially have to move the
  buffer to a place where there is enough space, freeing up the old
  place and allocating a new place. [1]
* In (5), we would then pass the original (now deallocated) buf
  pointer to kfree, leading to a double free.

If I were to describe the current interface of the hook
.report_fixup(dev, rdesc, rsize), it would be:

* report_fixup may modify rdesc[0] to rdesc[rsize-1]
* report_fixup may *not* deallocate rdesc
  (ownership of rdesc stays with the caller)
  * specifically, it may also not reallocate rdesc
    (because that may imply a deallocation)
* report_fixup returns a pointer to a buffer for which it can
  guarantee that it lives long enough for the kmemdup in (4), but
  which will *not* be deallocated by the caller (see (b) above).  The
  three techniques I have found for that are:
  * returning a subsection of the rdesc that it received
  * returning a pointer into a statically allocated array
    (e.g. motion_fixup() and ps3remote_fixup() in hid-sony.c)
  * allocating it with a devm_*() function.  My understanding was
    that this ties it to the lifetime of the device.  (e.g. the
    QUIRK_G752_KEYBOARD case in hid-asus.c)

Honestly, I still think that this reasoning holds, but I am happy to
be convinced otherwise.  Please let me know what you think.

—Günther

Re: [PATCH 3/3] HID: asus: avoid memory leak in asus_report_fixup()

[Günther Noack]

Hello!

On Tue, Feb 17, 2026 at 07:31:23PM +0100, Benjamin Tissoires wrote:
> On Feb 17 2026, Günther Noack wrote:
> > The asus_report_fixup() function was allocating a new buffer with kmemdup()
> > when growing the report descriptor but never freeing it.  Switch to
> > devm_kzalloc() to ensure the memory is managed and freed automatically when
> > the device is removed.
> 
> Actually this one is even worse: you can't use devm_kzalloc because
> hid-core.c will later call kfree(dev->rdesc) if dev->rdesc is different
> from the one provided by the low level driver. So we are going to have
> a double free.

The buffer returned by report_fixup() is duplicated first before
hid-core stores it in dev->rdesc.  The pointer that report_fixup()
returns is not managed by the caller.

I elaborated in the response to the other patch in [1].  You can see
it in the source code in the position marked with (4).

[1] https://lore.kernel.org/all/aZTEnPEHcWEkoTJR@google.com/


> I really wonder if this was ever tested.

I only convinced myself by staring at the code, because I do not
happen to have the matching USB devices here.  What it your usual
approach to verifying such changes?  raw-gadget?

—Günther

Re: [PATCH 0/3] HID: Fix some memory leaks in drivers/hid

[Günther Noack]

Hello!

On Tue, Feb 17, 2026 at 07:36:46PM +0100, Benjamin Tissoires wrote:
> On Feb 17 2026, Günther Noack wrote:
> > These patches fix a few memory leaks in HID report descriptor fixups.
> > 
> > FWIW, a good ad-hoc way to look for usages of allocation functions in
> > these is:
> > 
> >   awk '/static.*report_fixup.*/,/^}/ { print FILENAME, $0 }' drivers/hid/hid-*.c \
> >     | grep -E '(malloc|kzalloc|kcalloc|kmemdup)'
> > 
> > The devm_* variants are safe in this context, because they tie the
> > allocated memory to the lifetime of the driver.
> 
> No. Look at hid_close_report() in drivers/hid/hid-core.c.
> 
> HID still hasn't fully migrated to devm, so as a rule of thumb, if you
> change a kzalloc into a devm_kzalloc, you are getting into troubles
> unless you fix the all the kfree path.

OK, I have not verified where the devm-allocated objects get freed up.
If devm_*() is not possible here, then the drivers hid-asus.c and
hid-gembird.c have two additional memory leaks, because they do that.

$ awk '/static.*report_fixup.*/,/^}/ { print FILENAME, $0 }' drivers/hid/hid-*.c | grep -E 'alloc'
drivers/hid/hid-asus.c          new_rdesc = devm_kzalloc(&hdev->dev, new_size, GFP_KERNEL);
drivers/hid/hid-gembird.c               new_rdesc = devm_kzalloc(&hdev->dev, new_size, GFP_KERNEL);

(That is without my threee patches)


> > For transparency, I generated these commits with Gemini-CLI,
> > starting with this prompt:
> > 
> >     We are working in the Linux kernel. In the HID drivers in
> >     `drivers/hid/hid-*.c`, the `report_fixup` driver hook is a function
> >     that gets a byte buffer (with size) as input and that may modify that
> >     byte buffer, and optionally return a pointer to a new byte buffer and
> >     update the size.  The returned value is *not* memory-managed by the
> >     caller though and will not be freed subsequently.  When the
> 
> If the memory is *not* managed, why would gemini converts kzalloc into
> devm variants without changing the kfree paths????

I'm not sure I understand the question, it's not clear to me what you
mean by the "kfree paths".

I have seen usages of devm in other HID drivers and I was under the
impression that devm_* allocations would work in the HID subsystem to
allocate objects which are then freed automatically at a later point
when the device gets removed.  Is that inaccurate?


> >     `report_fixup` implementation allocates a new buffer and returns that,
> >     that will not get freed by the caller.  
> 
> This is wrong. See hid_close_report(): if the new rdesc (after fixup)
> differs from the one initially set, there is an explicit call to
> kfree().
> 
> -> there is no memleak AFAICT, and your prompt is wrong.

See my discussion in [1].  The pointer returned by report_fixup() is
immediately discarded in the position marked with (4).  This is still
in the hid_open_report() function where the leak happens.

Let me know whether this makes sense.  I'm happy to be corrected, but
so far, I still have the feeling that my reasoning is sound.

—Günther

[1] https://lore.kernel.org/all/aZTEnPEHcWEkoTJR@google.com/

Re: [PATCH 1/3] HID: apple: avoid memory leak in apple_report_fixup()

[Benjamin Tissoires]

On Feb 17 2026, Günther Noack wrote:
> Hello Benjamin!
> 
> On Tue, Feb 17, 2026 at 07:22:20PM +0100, Benjamin Tissoires wrote:
> > On Feb 17 2026, Günther Noack wrote:
> > > The apple_report_fixup() function was allocating a new buffer with
> > > kmemdup() but never freeing it. Since the caller of report_fixup() already
> > > provides a writable buffer and allows returning a pointer within that
> > > buffer, we can just modify the descriptor in-place and return the adjusted
> > > pointer.
> > > 
> > > Assisted-by: Gemini-CLI:Google Gemini 3
> > > Signed-off-by: Günther Noack <gnoack@google.com>
> > > ---
> > >  drivers/hid/hid-apple.c | 4 +---
> > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
> > > index 233e367cce1d..894adc23367b 100644
> > > --- a/drivers/hid/hid-apple.c
> > > +++ b/drivers/hid/hid-apple.c
> > > @@ -686,9 +686,7 @@ static const __u8 *apple_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> > >  		hid_info(hdev,
> > >  			 "fixing up Magic Keyboard battery report descriptor\n");
> > >  		*rsize = *rsize - 1;
> > > -		rdesc = kmemdup(rdesc + 1, *rsize, GFP_KERNEL);
> > > -		if (!rdesc)
> > > -			return NULL;
> > > +		rdesc = rdesc + 1;
> > 
> > I might be wrong, but later we call free(dev->rdesc) on device removal.
> > AFAICT, incrementing the pointer is undefined behavior.
> > 
> > What we should do instead is probably a krealloc instead of a kmemdup.
> > 
> > Same for all 3 patches.
> 
> Thanks for the review.
> 
> Let me try to address your three responses in one reply.
> 
> I am happy to accept it if I am wrong about this, but I don't
> understand your reasoning.  (This should go without saying, but maybe
> is worth reiterating, I would not have sent this if I had not
> convinced myself independently of LLM-assisted reasoning.)
> 
> Let me explain my reasoning based on the place where .report_fixup()
> is called from, which is hid_open_report() in hid-core.c:
> 
> 
> 	start = device->bpf_rdesc;                         /* (1) */
> 	if (WARN_ON(!start))
> 		return -ENODEV;
> 	size = device->bpf_rsize;
> 
> 	if (device->driver->report_fixup) {
> 		/*
> 		 * device->driver->report_fixup() needs to work
> 		 * on a copy of our report descriptor so it can
> 		 * change it.
> 		 */
> 		__u8 *buf = kmemdup(start, size, GFP_KERNEL);   /* (2) */
> 
> 		if (buf == NULL)
> 			return -ENOMEM;
> 
> 		start = device->driver->report_fixup(device, buf, &size);  /* (3) */
> 
> 		/*
> 		 * The second kmemdup is required in case report_fixup() returns
> 		 * a static read-only memory, but we have no idea if that memory
> 		 * needs to be cleaned up or not at the end.
> 		 */
> 		start = kmemdup(start, size, GFP_KERNEL);  /* (4) */
> 		kfree(buf);                                /* (5) */
> 		if (start == NULL)
> 			return -ENOMEM;
> 	}
> 
> 	device->rdesc = start;
> 	device->rsize = size;
> 
> 
> This function uses a slightly elaborate scheme to call .report_fixup:
> 
> (1) start is assigned to the original device->bpf_rdesc
> (2) buf is assigned to a copy of the 'start' buffer (deallocated in (5)).
>  .  It is done because buf is meant to be mutated by .report_fixup()
>  .  (3) start = ...->report_fixup(..., buf, ...)
>  .  (4) start = kmemdup(start, ...)
> (5) deallocate buf

Ouch. Yeah, sorry. I wrote that code and it seemed I completely paged
it out. Your code is actually correct (all three) but it would be nice
to have a longer commit message explaining this above.

The main point of this alloc before calling fixup is because some
drivers are using a static array as the new report descriptor. So we can
not free it later on. Working on a known copy allows to handle the kfree
correctly.

So yes, sorry, returning rdesc+1 in 1/3 and 2/3 is correct, and using a
devm_kzalloc is too in 3/3.

Cheers,
Benjamin

> 
> Importantly:
> 
> (a) The buffer buf passed to report_fixup() is a copy of the report
>     descriptor whose lifetime spans only from (2) to (5).
> (b) The result of .report_fixup(), start, is immediately discarded in
>     (4) and reassigned to the start variable again.
> 
> From (b), we can see that the result of .report_fixup() does *not* get
> deallocated by the caller, and thus, when the driver wants to return a
> newly allocated array, is must also hold a reference to it so that it
> can deallocate it later.
> 
> From (a), we can see that the report_fixup hook is free to manipulate
> the contents of the buffer that it receives, but if we were to
> *reallocate* it within report_fixup, as you are suggesting above, it
> could become a double free:
> 
> * During realloc, the allocator would potentially have to move the
>   buffer to a place where there is enough space, freeing up the old
>   place and allocating a new place. [1]
> * In (5), we would then pass the original (now deallocated) buf
>   pointer to kfree, leading to a double free.
> 
> If I were to describe the current interface of the hook
> .report_fixup(dev, rdesc, rsize), it would be:
> 
> * report_fixup may modify rdesc[0] to rdesc[rsize-1]
> * report_fixup may *not* deallocate rdesc
>   (ownership of rdesc stays with the caller)
>   * specifically, it may also not reallocate rdesc
>     (because that may imply a deallocation)
> * report_fixup returns a pointer to a buffer for which it can
>   guarantee that it lives long enough for the kmemdup in (4), but
>   which will *not* be deallocated by the caller (see (b) above).  The
>   three techniques I have found for that are:
>   * returning a subsection of the rdesc that it received
>   * returning a pointer into a statically allocated array
>     (e.g. motion_fixup() and ps3remote_fixup() in hid-sony.c)
>   * allocating it with a devm_*() function.  My understanding was
>     that this ties it to the lifetime of the device.  (e.g. the
>     QUIRK_G752_KEYBOARD case in hid-asus.c)
> 
> Honestly, I still think that this reasoning holds, but I am happy to
> be convinced otherwise.  Please let me know what you think.
> 
> —Günther

Re: [PATCH 1/3] HID: apple: avoid memory leak in apple_report_fixup()

[Günther Noack]

On Wed, Feb 18, 2026 at 08:04:52PM +0100, Benjamin Tissoires wrote:
> Ouch. Yeah, sorry. I wrote that code and it seemed I completely paged
> it out. Your code is actually correct (all three) but it would be nice
> to have a longer commit message explaining this above.
> 
> The main point of this alloc before calling fixup is because some
> drivers are using a static array as the new report descriptor. So we can
> not free it later on. Working on a known copy allows to handle the kfree
> correctly.
> 
> So yes, sorry, returning rdesc+1 in 1/3 and 2/3 is correct, and using a
> devm_kzalloc is too in 3/3.

Ah OK, thanks for the review!  I sent you an updated version where I
am trying to express it more clearly in the commit messages.

I also added a commit that documents the expected allocation
properties in hid.h where the .report_fixup() field is defined (feel
free to ignore this one, if it feels like it's too much).

Link to V2:
https://lore.kernel.org/all/20260219154338.786625-2-gnoack@google.com/

—Günther