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

["Günther Noack" <gnoack@google.com>]

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