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-googOn 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 >
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
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
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
© 2016 - 2026 Red Hat, Inc.