[PATCH] fbdev: udlfb: validate vendor descriptor items

[PATCH] fbdev: udlfb: validate vendor descriptor items

[Pengpeng Hou]

dlfb_parse_vendor_descriptor() walks key-length-value items inside the
DisplayLink vendor descriptor.

Require each item to contain its key, length and declared value bytes
before reading item-specific fields such as max_area.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/video/fbdev/udlfb.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/video/fbdev/udlfb.c b/drivers/video/fbdev/udlfb.c
index fdbb8671a810..e78d6f95c9c5 100644
--- a/drivers/video/fbdev/udlfb.c
+++ b/drivers/video/fbdev/udlfb.c
@@ -1586,19 +1586,29 @@ static int dlfb_parse_vendor_descriptor(struct dlfb_data *dlfb,
 		desc += 5; /* the fixed header we've already parsed */
 
 		while (desc < desc_end) {
+			char *value;
 			u8 length;
 			u16 key;
 
-			key = *desc++;
-			key |= (u16)*desc++ << 8;
+			if (desc_end - desc < sizeof(key) + sizeof(length))
+				goto unrecognized;
+
+			key = get_unaligned_le16(desc);
+			desc += sizeof(key);
 			length = *desc++;
 
+			if (length > desc_end - desc)
+				goto unrecognized;
+
+			value = desc;
 			switch (key) {
 			case 0x0200: { /* max_area */
-				u32 max_area = *desc++;
-				max_area |= (u32)*desc++ << 8;
-				max_area |= (u32)*desc++ << 16;
-				max_area |= (u32)*desc++ << 24;
+				u32 max_area;
+
+				if (length < sizeof(max_area))
+					goto unrecognized;
+
+				max_area = get_unaligned_le32(value);
 				dev_warn(&intf->dev,
 					 "DL chip limited to %d pixel modes\n",
 					 max_area);
-- 
2.43.0

Re: [PATCH] fbdev: udlfb: validate vendor descriptor items

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Out-of-bounds read in `dev_info` using `%11ph` format specifier, potentially causing a kernel panic.
--

commit 8284c1b4f9c66a4ef2393d5e8aa1741e7ef14d8a
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

fbdev: udlfb: validate vendor descriptor items

This commit validates key-length-value items inside the DisplayLink vendor
descriptor in dlfb_parse_vendor_descriptor(). It ensures each item contains
its key, length, and declared value bytes before reading item-specific fields.

> diff --git a/drivers/video/fbdev/udlfb.c b/drivers/video/fbdev/udlfb.c
> index fdbb8671a810c..e78d6f95c9c5c 100644
> --- a/drivers/video/fbdev/udlfb.c
> +++ b/drivers/video/fbdev/udlfb.c
> @@ -1586,19 +1586,29 @@ static int dlfb_parse_vendor_descriptor(struct dlfb_data *dlfb,

[Severity: High]
This isn't a bug introduced by this patch, but earlier in
dlfb_parse_vendor_descriptor(), there is a potential out-of-bounds read:

	if (total_len > 5) {
		dev_info(&intf->dev,
			 "vendor descriptor length: %d data: %11ph\n",
			 total_len, desc);

Can this unconditionally read 11 bytes even if total_len is as small as 6?

If a malicious USB device stalls the initial control request for the vendor
descriptor, and instead provides it as a 6-byte extra descriptor within the
configuration descriptor blob, desc will point directly into the USB core's
rawdescriptors allocation, and total_len will be 6.

Because the %11ph format specifier unconditionally reads exactly 11 bytes,
would this read 5 bytes out of bounds, potentially causing a kernel panic if
the buffer ends at a page boundary?

>  		desc += 5; /* the fixed header we've already parsed */
>  
>  		while (desc < desc_end) {

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706093038.80131-1-pengpeng@iscas.ac.cn?part=1

Re: [PATCH] fbdev: udlfb: validate vendor descriptor items

[Helge Deller]

On 7/6/26 11:30, Pengpeng Hou wrote:
> dlfb_parse_vendor_descriptor() walks key-length-value items inside the
> DisplayLink vendor descriptor.
> 
> Require each item to contain its key, length and declared value bytes
> before reading item-specific fields such as max_area.
> 
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
>   drivers/video/fbdev/udlfb.c | 22 ++++++++++++++++------
>   1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/video/fbdev/udlfb.c b/drivers/video/fbdev/udlfb.c
> index fdbb8671a810..e78d6f95c9c5 100644
> --- a/drivers/video/fbdev/udlfb.c
> +++ b/drivers/video/fbdev/udlfb.c
> @@ -1586,19 +1586,29 @@ static int dlfb_parse_vendor_descriptor(struct dlfb_data *dlfb,
>   		desc += 5; /* the fixed header we've already parsed */
>   
>   		while (desc < desc_end) {
> +			char *value;
>   			u8 length;
>   			u16 key;
>   
> -			key = *desc++;
> -			key |= (u16)*desc++ << 8;
> +			if (desc_end - desc < sizeof(key) + sizeof(length))
> +				goto unrecognized;
> +
> +			key = get_unaligned_le16(desc);

Is there a reason why you switch to unconditional little-endian reads?
Is this "vendor descriptor" always little-endian?
If yes, then your patch is probably correct.
If not, I think your patch will most likely break big-endian machines.

Helge

> +			desc += sizeof(key);
>   			length = *desc++;
>   
> +			if (length > desc_end - desc)
> +				goto unrecognized;
> +
> +			value = desc;
>   			switch (key) {
>   			case 0x0200: { /* max_area */
> -				u32 max_area = *desc++;
> -				max_area |= (u32)*desc++ << 8;
> -				max_area |= (u32)*desc++ << 16;
> -				max_area |= (u32)*desc++ << 24;
> +				u32 max_area;
> +
> +				if (length < sizeof(max_area))
> +					goto unrecognized;
> +
> +				max_area = get_unaligned_le32(value);
>   				dev_warn(&intf->dev,
>   					 "DL chip limited to %d pixel modes\n",
>   					 max_area);

Re: [PATCH] fbdev: udlfb: validate vendor descriptor items

[Helge Deller]

On 7/18/26 20:26, Helge Deller wrote:
> On 7/6/26 11:30, Pengpeng Hou wrote:
>> dlfb_parse_vendor_descriptor() walks key-length-value items inside the
>> DisplayLink vendor descriptor.
>>
>> Require each item to contain its key, length and declared value bytes
>> before reading item-specific fields such as max_area.
>>
>> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
>> ---
>>   drivers/video/fbdev/udlfb.c | 22 ++++++++++++++++------
>>   1 file changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/video/fbdev/udlfb.c b/drivers/video/fbdev/udlfb.c
>> index fdbb8671a810..e78d6f95c9c5 100644
>> --- a/drivers/video/fbdev/udlfb.c
>> +++ b/drivers/video/fbdev/udlfb.c
>> @@ -1586,19 +1586,29 @@ static int dlfb_parse_vendor_descriptor(struct dlfb_data *dlfb,
>>           desc += 5; /* the fixed header we've already parsed */
>>           while (desc < desc_end) {
>> +            char *value;
>>               u8 length;
>>               u16 key;
>> -            key = *desc++;
>> -            key |= (u16)*desc++ << 8;
>> +            if (desc_end - desc < sizeof(key) + sizeof(length))
>> +                goto unrecognized;
>> +
>> +            key = get_unaligned_le16(desc);
> 
> Is there a reason why you switch to unconditional little-endian reads?
> Is this "vendor descriptor" always little-endian?
> If yes, then your patch is probably correct.
> If not, I think your patch will most likely break big-endian machines.
Please ignore my comments above.
I should have looked more closely.
Your patch is of course correct!

The patch is now added to fbdev git tree.

Thanks!
Helge