[PATCH 1/1] HID: logitech-dj: Prevent REPORT_ID_DJ_SHORT related user initiated OOB write

[PATCH 1/1] HID: logitech-dj: Prevent REPORT_ID_DJ_SHORT related user initiated OOB write

[Lee Jones]

logi_dj_recv_send_report() assumes that all incoming REPORT_ID_DJ_SHORT
reports are 14 Bytes (DJREPORT_SHORT_LENGTH - 1) long.  It uses that
assumption to load the associated field's 'value' array with 14 Bytes of
data.  However, if a malicious user only sends say 1 Byte of data,
'report_count' will be 1 and only 1 Byte of memory will be allocated to
the 'value' Byte array.  When we come to populate 'value[1-13]' we will
experience an OOB write.

Signed-off-by: Lee Jones <lee@kernel.org>
---
 drivers/hid/hid-logitech-dj.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index 44b716697510..885b986c7a12 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -1282,6 +1282,12 @@ static int logi_dj_recv_send_report(struct dj_receiver_dev *djrcv_dev,
 		return -ENODEV;
 	}
 
+	if (report->maxfield < 1 || report->field[0]->report_count != DJREPORT_SHORT_LENGTH - 1) {
+		hid_err(hdev, "Expected size of dj report is %d, but got %d",
+			DJREPORT_SHORT_LENGTH - 1, report->field[0]->report_count);
+		return -EINVAL;
+	}
+
 	for (i = 0; i < DJREPORT_SHORT_LENGTH - 1; i++)
 		report->field[0]->value[i] = data[i];
 
-- 
2.53.0.851.ga537e3e6e9-goog

Re: [PATCH 1/1] HID: logitech-dj: Prevent REPORT_ID_DJ_SHORT related user initiated OOB write

[Benjamin Tissoires]

On Mar 17 2026, Lee Jones wrote:
> logi_dj_recv_send_report() assumes that all incoming REPORT_ID_DJ_SHORT
> reports are 14 Bytes (DJREPORT_SHORT_LENGTH - 1) long.  It uses that
> assumption to load the associated field's 'value' array with 14 Bytes of
> data.  However, if a malicious user only sends say 1 Byte of data,
> 'report_count' will be 1 and only 1 Byte of memory will be allocated to
> the 'value' Byte array.  When we come to populate 'value[1-13]' we will
> experience an OOB write.
> 
> Signed-off-by: Lee Jones <lee@kernel.org>
> ---
>  drivers/hid/hid-logitech-dj.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> index 44b716697510..885b986c7a12 100644
> --- a/drivers/hid/hid-logitech-dj.c
> +++ b/drivers/hid/hid-logitech-dj.c
> @@ -1282,6 +1282,12 @@ static int logi_dj_recv_send_report(struct dj_receiver_dev *djrcv_dev,
>  		return -ENODEV;
>  	}
>  
> +	if (report->maxfield < 1 || report->field[0]->report_count != DJREPORT_SHORT_LENGTH - 1) {

This is all static information. So this should be checked in the
.probe(), once the device has been parsed, not for every single call of
logi_dj_recv_send_report().

Cheers,
Benjamin

> +		hid_err(hdev, "Expected size of dj report is %d, but got %d",
> +			DJREPORT_SHORT_LENGTH - 1, report->field[0]->report_count);
> +		return -EINVAL;
> +	}
> +
>  	for (i = 0; i < DJREPORT_SHORT_LENGTH - 1; i++)
>  		report->field[0]->value[i] = data[i];
>  
> -- 
> 2.53.0.851.ga537e3e6e9-goog
> 
> 

Re: [PATCH 1/1] HID: logitech-dj: Prevent REPORT_ID_DJ_SHORT related user initiated OOB write

[Lee Jones]

On Tue, 17 Mar 2026, Benjamin Tissoires wrote:

> On Mar 17 2026, Lee Jones wrote:
> > logi_dj_recv_send_report() assumes that all incoming REPORT_ID_DJ_SHORT
> > reports are 14 Bytes (DJREPORT_SHORT_LENGTH - 1) long.  It uses that
> > assumption to load the associated field's 'value' array with 14 Bytes of
> > data.  However, if a malicious user only sends say 1 Byte of data,
> > 'report_count' will be 1 and only 1 Byte of memory will be allocated to
> > the 'value' Byte array.  When we come to populate 'value[1-13]' we will
> > experience an OOB write.
> > 
> > Signed-off-by: Lee Jones <lee@kernel.org>
> > ---
> >  drivers/hid/hid-logitech-dj.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> > index 44b716697510..885b986c7a12 100644
> > --- a/drivers/hid/hid-logitech-dj.c
> > +++ b/drivers/hid/hid-logitech-dj.c
> > @@ -1282,6 +1282,12 @@ static int logi_dj_recv_send_report(struct dj_receiver_dev *djrcv_dev,
> >  		return -ENODEV;
> >  	}
> >  
> > +	if (report->maxfield < 1 || report->field[0]->report_count != DJREPORT_SHORT_LENGTH - 1) {
> 
> This is all static information. So this should be checked in the
> .probe(), once the device has been parsed, not for every single call of
> logi_dj_recv_send_report().

Doesn't report_count come from the device?

I can manipulate it with my user app.

-- 
Lee Jones [李琼斯]

Re: [PATCH 1/1] HID: logitech-dj: Prevent REPORT_ID_DJ_SHORT related user initiated OOB write

[Jiri Kosina]

On Tue, 17 Mar 2026, Lee Jones wrote:

> > > diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> > > index 44b716697510..885b986c7a12 100644
> > > --- a/drivers/hid/hid-logitech-dj.c
> > > +++ b/drivers/hid/hid-logitech-dj.c
> > > @@ -1282,6 +1282,12 @@ static int logi_dj_recv_send_report(struct dj_receiver_dev *djrcv_dev,
> > >  		return -ENODEV;
> > >  	}
> > >  
> > > +	if (report->maxfield < 1 || report->field[0]->report_count != DJREPORT_SHORT_LENGTH - 1) {
> > 
> > This is all static information. So this should be checked in the
> > .probe(), once the device has been parsed, not for every single call of
> > logi_dj_recv_send_report().
> 
> Doesn't report_count come from the device?

The point is -- maxfield and report_count can't change once parsed unless 
the report descriptor would be re-read and re-parsed (which doesn't happen 
in runtime, only during probe).

So checking during probe/parse time just once should be sufficient, 
instead of checking it upon every received report.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 1/1] HID: logitech-dj: Prevent REPORT_ID_DJ_SHORT related user initiated OOB write

[Lee Jones]

On Tue, 17 Mar 2026, Jiri Kosina wrote:

> On Tue, 17 Mar 2026, Lee Jones wrote:
> 
> > > > diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> > > > index 44b716697510..885b986c7a12 100644
> > > > --- a/drivers/hid/hid-logitech-dj.c
> > > > +++ b/drivers/hid/hid-logitech-dj.c
> > > > @@ -1282,6 +1282,12 @@ static int logi_dj_recv_send_report(struct dj_receiver_dev *djrcv_dev,
> > > >  		return -ENODEV;
> > > >  	}
> > > >  
> > > > +	if (report->maxfield < 1 || report->field[0]->report_count != DJREPORT_SHORT_LENGTH - 1) {
> > > 
> > > This is all static information. So this should be checked in the
> > > .probe(), once the device has been parsed, not for every single call of
> > > logi_dj_recv_send_report().
> > 
> > Doesn't report_count come from the device?
> 
> The point is -- maxfield and report_count can't change once parsed unless 
> the report descriptor would be re-read and re-parsed (which doesn't happen 
> in runtime, only during probe).
> 
> So checking during probe/parse time just once should be sufficient, 
> instead of checking it upon every received report.

Okay, thanks for the explanation.  I'll give it a shot.

-- 
Lee Jones [李琼斯]