diff mbox series

usb: typec: ucsi: Fix connector status writing past buffer size

Message ID 20241203200010.2821132-1-lucas.demarchi@intel.com (mailing list archive)
State Accepted
Commit 33ead7e538183b1348ba60af90027240a10de751
Headers show
Series usb: typec: ucsi: Fix connector status writing past buffer size | expand

Commit Message

Lucas De Marchi Dec. 3, 2024, 8 p.m. UTC
Similar to commit 65c4c9447bfc ("usb: typec: ucsi: Fix a missing bits to
bytes conversion in ucsi_init()"), there was a missing conversion from
bits to bytes. Here the outcome is worse though: since the value is
lower than UCSI_MAX_DATA_LENGTH, instead of bailing out with an error,
it writes past the buffer size.

The error is then seen in other places like below:

	Oops: general protection fault, probably for non-canonical address 0x891e812cd0ed968: 0000 [#1] PREEMPT SMP NOPTI
	CPU: 3 UID: 110 PID: 906 Comm: prometheus-node Not tainted 6.13.0-rc1-xe #1
	Hardware name: Intel Corporation Lunar Lake Client Platform/LNL-M LP5 RVP1, BIOS LNLMFWI1.R00.3222.D84.2410171025 10/17/2024
	RIP: 0010:power_supply_get_property+0x3e/0xe0
	Code: 85 c0 7e 4f 4c 8b 07 89 f3 49 89 d4 49 8b 48 20 48 85 c9 74 72 49 8b 70 18 31 d2 31 c0 eb 0b 83 c2 01 48 63 c2 48 39 c8 73 5d <3b> 1c 86 75 f0 49 8b 40 28 4c 89 e2 89 de ff d0 0f 1f 00 5b 41 5c
	RSP: 0018:ffffc900017dfa50 EFLAGS: 00010246
	RAX: 0000000000000000 RBX: 0000000000000011 RCX: c963b02c06092008
	RDX: 0000000000000000 RSI: 0891e812cd0ed968 RDI: ffff888121dd6800
	RBP: ffffc900017dfa68 R08: ffff88810a4024b8 R09: 0000000000000000
	R10: 0000000000000000 R11: 0000000000000000 R12: ffffc900017dfa78
	R13: ffff888121dd6800 R14: ffff888138ad2c00 R15: ffff88810c57c528
	FS:  00007713a2ffd6c0(0000) GS:ffff88846f380000(0000) knlGS:0000000000000000
	CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
	CR2: 000000c0004b1000 CR3: 0000000121ce8003 CR4: 0000000000f72ef0
	DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
	DR3: 0000000000000000 DR6: 00000000ffff07f0 DR7: 0000000000000400
	PKRU: 55555554
	Call Trace:
	 <TASK>
	 ? show_regs+0x6c/0x80
	 ? die_addr+0x37/0xa0
	 ? exc_general_protection+0x1c1/0x440
	 ? asm_exc_general_protection+0x27/0x30
	 ? power_supply_get_property+0x3e/0xe0
	 power_supply_hwmon_read+0x50/0xe0
	 hwmon_attr_show+0x46/0x170
	 dev_attr_show+0x1a/0x70
	 sysfs_kf_seq_show+0xaa/0x120
	 kernfs_seq_show+0x41/0x60

Just use the buffer size as argument to fix it.

Fixes: 226ff2e681d0 ("usb: typec: ucsi: Convert connector specific commands to bitmaps")
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/usb/typec/ucsi/ucsi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Sebastian Reichel Dec. 4, 2024, 12:14 a.m. UTC | #1
Hi,

On Tue, Dec 03, 2024 at 12:00:10PM -0800, Lucas De Marchi wrote:
> Similar to commit 65c4c9447bfc ("usb: typec: ucsi: Fix a missing bits to
> bytes conversion in ucsi_init()"), there was a missing conversion from
> bits to bytes. Here the outcome is worse though: since the value is
> lower than UCSI_MAX_DATA_LENGTH, instead of bailing out with an error,
> it writes past the buffer size.
> 
> The error is then seen in other places like below:
> 
> 	Oops: general protection fault, probably for non-canonical address 0x891e812cd0ed968: 0000 [#1] PREEMPT SMP NOPTI
> 	CPU: 3 UID: 110 PID: 906 Comm: prometheus-node Not tainted 6.13.0-rc1-xe #1
> 	Hardware name: Intel Corporation Lunar Lake Client Platform/LNL-M LP5 RVP1, BIOS LNLMFWI1.R00.3222.D84.2410171025 10/17/2024
> 	RIP: 0010:power_supply_get_property+0x3e/0xe0
> 	Code: 85 c0 7e 4f 4c 8b 07 89 f3 49 89 d4 49 8b 48 20 48 85 c9 74 72 49 8b 70 18 31 d2 31 c0 eb 0b 83 c2 01 48 63 c2 48 39 c8 73 5d <3b> 1c 86 75 f0 49 8b 40 28 4c 89 e2 89 de ff d0 0f 1f 00 5b 41 5c
> 	RSP: 0018:ffffc900017dfa50 EFLAGS: 00010246
> 	RAX: 0000000000000000 RBX: 0000000000000011 RCX: c963b02c06092008
> 	RDX: 0000000000000000 RSI: 0891e812cd0ed968 RDI: ffff888121dd6800
> 	RBP: ffffc900017dfa68 R08: ffff88810a4024b8 R09: 0000000000000000
> 	R10: 0000000000000000 R11: 0000000000000000 R12: ffffc900017dfa78
> 	R13: ffff888121dd6800 R14: ffff888138ad2c00 R15: ffff88810c57c528
> 	FS:  00007713a2ffd6c0(0000) GS:ffff88846f380000(0000) knlGS:0000000000000000
> 	CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> 	CR2: 000000c0004b1000 CR3: 0000000121ce8003 CR4: 0000000000f72ef0
> 	DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> 	DR3: 0000000000000000 DR6: 00000000ffff07f0 DR7: 0000000000000400
> 	PKRU: 55555554
> 	Call Trace:
> 	 <TASK>
> 	 ? show_regs+0x6c/0x80
> 	 ? die_addr+0x37/0xa0
> 	 ? exc_general_protection+0x1c1/0x440
> 	 ? asm_exc_general_protection+0x27/0x30
> 	 ? power_supply_get_property+0x3e/0xe0
> 	 power_supply_hwmon_read+0x50/0xe0
> 	 hwmon_attr_show+0x46/0x170
> 	 dev_attr_show+0x1a/0x70
> 	 sysfs_kf_seq_show+0xaa/0x120
> 	 kernfs_seq_show+0x41/0x60
> 
> Just use the buffer size as argument to fix it.
> 
> Fixes: 226ff2e681d0 ("usb: typec: ucsi: Convert connector specific commands to bitmaps")
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>

Also you are missing at some tags:

Reported-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
Closes: https://lore.kernel.org/all/SJ1PR11MB6129CCD82CD78D8EE6E27EF4B9362@SJ1PR11MB6129.namprd11.prod.outlook.com/

and considering he figured out the root cause (and there is no
better tag AFAIK):

Suggested-by: Thomas Weißschuh <linux@weissschuh.net>

Greetings,

-- Sebastian

> ---
>  drivers/usb/typec/ucsi/ucsi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index c435c0835744a..69caae84879e9 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -651,7 +651,8 @@ static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient)
>  static int ucsi_get_connector_status(struct ucsi_connector *con, bool conn_ack)
>  {
>  	u64 command = UCSI_GET_CONNECTOR_STATUS | UCSI_CONNECTOR_NUMBER(con->num);
> -	size_t size = min(UCSI_GET_CONNECTOR_STATUS_SIZE, UCSI_MAX_DATA_LENGTH(con->ucsi));
> +	size_t size = min(sizeof(con->status),
> +			  UCSI_MAX_DATA_LENGTH(con->ucsi));
>  	int ret;
>  
>  	ret = ucsi_send_command_common(con->ucsi, command, &con->status, size, conn_ack);
> -- 
> 2.47.0
> 
>
Chaitanya Kumar Borah Dec. 4, 2024, 5:47 a.m. UTC | #2
> -----Original Message-----
> From: De Marchi, Lucas <lucas.demarchi@intel.com>
> Sent: Wednesday, December 4, 2024 1:30 AM
> To: linux-usb@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>; Heikki Krogerus
> <heikki.krogerus@linux.intel.com>; intel-xe@lists.freedesktop.org; intel-
> gfx@lists.freedesktop.org; Thomas Weißschuh <linux@weissschuh.net>;
> Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com>; Luca Coelho
> <luca@coelho.fi>; De Marchi, Lucas <lucas.demarchi@intel.com>
> Subject: [PATCH] usb: typec: ucsi: Fix connector status writing past buffer size
> 
> Similar to commit 65c4c9447bfc ("usb: typec: ucsi: Fix a missing bits to bytes
> conversion in ucsi_init()"), there was a missing conversion from bits to bytes.
> Here the outcome is worse though: since the value is lower than
> UCSI_MAX_DATA_LENGTH, instead of bailing out with an error, it writes past
> the buffer size.
> 
> The error is then seen in other places like below:
> 
> 	Oops: general protection fault, probably for non-canonical address
> 0x891e812cd0ed968: 0000 [#1] PREEMPT SMP NOPTI
> 	CPU: 3 UID: 110 PID: 906 Comm: prometheus-node Not tainted
> 6.13.0-rc1-xe #1
> 	Hardware name: Intel Corporation Lunar Lake Client Platform/LNL-M
> LP5 RVP1, BIOS LNLMFWI1.R00.3222.D84.2410171025 10/17/2024
> 	RIP: 0010:power_supply_get_property+0x3e/0xe0
> 	Code: 85 c0 7e 4f 4c 8b 07 89 f3 49 89 d4 49 8b 48 20 48 85 c9 74 72
> 49 8b 70 18 31 d2 31 c0 eb 0b 83 c2 01 48 63 c2 48 39 c8 73 5d <3b> 1c 86 75
> f0 49 8b 40 28 4c 89 e2 89 de ff d0 0f 1f 00 5b 41 5c
> 	RSP: 0018:ffffc900017dfa50 EFLAGS: 00010246
> 	RAX: 0000000000000000 RBX: 0000000000000011 RCX:
> c963b02c06092008
> 	RDX: 0000000000000000 RSI: 0891e812cd0ed968 RDI:
> ffff888121dd6800
> 	RBP: ffffc900017dfa68 R08: ffff88810a4024b8 R09: 0000000000000000
> 	R10: 0000000000000000 R11: 0000000000000000 R12:
> ffffc900017dfa78
> 	R13: ffff888121dd6800 R14: ffff888138ad2c00 R15: ffff88810c57c528
> 	FS:  00007713a2ffd6c0(0000) GS:ffff88846f380000(0000)
> knlGS:0000000000000000
> 	CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> 	CR2: 000000c0004b1000 CR3: 0000000121ce8003 CR4:
> 0000000000f72ef0
> 	DR0: 0000000000000000 DR1: 0000000000000000 DR2:
> 0000000000000000
> 	DR3: 0000000000000000 DR6: 00000000ffff07f0 DR7:
> 0000000000000400
> 	PKRU: 55555554
> 	Call Trace:
> 	 <TASK>
> 	 ? show_regs+0x6c/0x80
> 	 ? die_addr+0x37/0xa0
> 	 ? exc_general_protection+0x1c1/0x440
> 	 ? asm_exc_general_protection+0x27/0x30
> 	 ? power_supply_get_property+0x3e/0xe0
> 	 power_supply_hwmon_read+0x50/0xe0
> 	 hwmon_attr_show+0x46/0x170
> 	 dev_attr_show+0x1a/0x70
> 	 sysfs_kf_seq_show+0xaa/0x120
> 	 kernfs_seq_show+0x41/0x60
> 
> Just use the buffer size as argument to fix it.
> 
> Fixes: 226ff2e681d0 ("usb: typec: ucsi: Convert connector specific commands
> to bitmaps")
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

Tested-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>

> ---
>  drivers/usb/typec/ucsi/ucsi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c index
> c435c0835744a..69caae84879e9 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -651,7 +651,8 @@ static void ucsi_unregister_altmodes(struct
> ucsi_connector *con, u8 recipient)  static int ucsi_get_connector_status(struct
> ucsi_connector *con, bool conn_ack)  {
>  	u64 command = UCSI_GET_CONNECTOR_STATUS |
> UCSI_CONNECTOR_NUMBER(con->num);
> -	size_t size = min(UCSI_GET_CONNECTOR_STATUS_SIZE,
> UCSI_MAX_DATA_LENGTH(con->ucsi));
> +	size_t size = min(sizeof(con->status),
> +			  UCSI_MAX_DATA_LENGTH(con->ucsi));
>  	int ret;
> 
>  	ret = ucsi_send_command_common(con->ucsi, command, &con-
> >status, size, conn_ack);
> --
> 2.47.0
Thomas Weißschuh Dec. 4, 2024, 6:17 a.m. UTC | #3
On 2024-12-03 12:00:10-0800, Lucas De Marchi wrote:
> Similar to commit 65c4c9447bfc ("usb: typec: ucsi: Fix a missing bits to
> bytes conversion in ucsi_init()"), there was a missing conversion from
> bits to bytes. Here the outcome is worse though: since the value is
> lower than UCSI_MAX_DATA_LENGTH, instead of bailing out with an error,
> it writes past the buffer size.
> 
> The error is then seen in other places like below:
> 
> 	Oops: general protection fault, probably for non-canonical address 0x891e812cd0ed968: 0000 [#1] PREEMPT SMP NOPTI
> 	CPU: 3 UID: 110 PID: 906 Comm: prometheus-node Not tainted 6.13.0-rc1-xe #1
> 	Hardware name: Intel Corporation Lunar Lake Client Platform/LNL-M LP5 RVP1, BIOS LNLMFWI1.R00.3222.D84.2410171025 10/17/2024
> 	RIP: 0010:power_supply_get_property+0x3e/0xe0
> 	Code: 85 c0 7e 4f 4c 8b 07 89 f3 49 89 d4 49 8b 48 20 48 85 c9 74 72 49 8b 70 18 31 d2 31 c0 eb 0b 83 c2 01 48 63 c2 48 39 c8 73 5d <3b> 1c 86 75 f0 49 8b 40 28 4c 89 e2 89 de ff d0 0f 1f 00 5b 41 5c
> 	RSP: 0018:ffffc900017dfa50 EFLAGS: 00010246
> 	RAX: 0000000000000000 RBX: 0000000000000011 RCX: c963b02c06092008
> 	RDX: 0000000000000000 RSI: 0891e812cd0ed968 RDI: ffff888121dd6800
> 	RBP: ffffc900017dfa68 R08: ffff88810a4024b8 R09: 0000000000000000
> 	R10: 0000000000000000 R11: 0000000000000000 R12: ffffc900017dfa78
> 	R13: ffff888121dd6800 R14: ffff888138ad2c00 R15: ffff88810c57c528
> 	FS:  00007713a2ffd6c0(0000) GS:ffff88846f380000(0000) knlGS:0000000000000000
> 	CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> 	CR2: 000000c0004b1000 CR3: 0000000121ce8003 CR4: 0000000000f72ef0
> 	DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> 	DR3: 0000000000000000 DR6: 00000000ffff07f0 DR7: 0000000000000400
> 	PKRU: 55555554
> 	Call Trace:
> 	 <TASK>
> 	 ? show_regs+0x6c/0x80
> 	 ? die_addr+0x37/0xa0
> 	 ? exc_general_protection+0x1c1/0x440
> 	 ? asm_exc_general_protection+0x27/0x30
> 	 ? power_supply_get_property+0x3e/0xe0
> 	 power_supply_hwmon_read+0x50/0xe0
> 	 hwmon_attr_show+0x46/0x170
> 	 dev_attr_show+0x1a/0x70
> 	 sysfs_kf_seq_show+0xaa/0x120
> 	 kernfs_seq_show+0x41/0x60
> 
> Just use the buffer size as argument to fix it.
> 
> Fixes: 226ff2e681d0 ("usb: typec: ucsi: Convert connector specific commands to bitmaps")
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/usb/typec/ucsi/ucsi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index c435c0835744a..69caae84879e9 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -651,7 +651,8 @@ static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient)
>  static int ucsi_get_connector_status(struct ucsi_connector *con, bool conn_ack)
>  {
>  	u64 command = UCSI_GET_CONNECTOR_STATUS | UCSI_CONNECTOR_NUMBER(con->num);
> -	size_t size = min(UCSI_GET_CONNECTOR_STATUS_SIZE, UCSI_MAX_DATA_LENGTH(con->ucsi));
> +	size_t size = min(sizeof(con->status),
> +			  UCSI_MAX_DATA_LENGTH(con->ucsi));

Why the linebreak?
The previous line was longer and all other lines are longer, too.

In any case:

Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>

>  	int ret;
>  
>  	ret = ucsi_send_command_common(con->ucsi, command, &con->status, size, conn_ack);
> -- 
> 2.47.0
>
Heikki Krogerus Dec. 4, 2024, 9:23 a.m. UTC | #4
On Tue, Dec 03, 2024 at 12:00:10PM -0800, Lucas De Marchi wrote:
> Similar to commit 65c4c9447bfc ("usb: typec: ucsi: Fix a missing bits to
> bytes conversion in ucsi_init()"), there was a missing conversion from
> bits to bytes. Here the outcome is worse though: since the value is
> lower than UCSI_MAX_DATA_LENGTH, instead of bailing out with an error,
> it writes past the buffer size.
> 
> The error is then seen in other places like below:
> 
> 	Oops: general protection fault, probably for non-canonical address 0x891e812cd0ed968: 0000 [#1] PREEMPT SMP NOPTI
> 	CPU: 3 UID: 110 PID: 906 Comm: prometheus-node Not tainted 6.13.0-rc1-xe #1
> 	Hardware name: Intel Corporation Lunar Lake Client Platform/LNL-M LP5 RVP1, BIOS LNLMFWI1.R00.3222.D84.2410171025 10/17/2024
> 	RIP: 0010:power_supply_get_property+0x3e/0xe0
> 	Code: 85 c0 7e 4f 4c 8b 07 89 f3 49 89 d4 49 8b 48 20 48 85 c9 74 72 49 8b 70 18 31 d2 31 c0 eb 0b 83 c2 01 48 63 c2 48 39 c8 73 5d <3b> 1c 86 75 f0 49 8b 40 28 4c 89 e2 89 de ff d0 0f 1f 00 5b 41 5c
> 	RSP: 0018:ffffc900017dfa50 EFLAGS: 00010246
> 	RAX: 0000000000000000 RBX: 0000000000000011 RCX: c963b02c06092008
> 	RDX: 0000000000000000 RSI: 0891e812cd0ed968 RDI: ffff888121dd6800
> 	RBP: ffffc900017dfa68 R08: ffff88810a4024b8 R09: 0000000000000000
> 	R10: 0000000000000000 R11: 0000000000000000 R12: ffffc900017dfa78
> 	R13: ffff888121dd6800 R14: ffff888138ad2c00 R15: ffff88810c57c528
> 	FS:  00007713a2ffd6c0(0000) GS:ffff88846f380000(0000) knlGS:0000000000000000
> 	CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> 	CR2: 000000c0004b1000 CR3: 0000000121ce8003 CR4: 0000000000f72ef0
> 	DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> 	DR3: 0000000000000000 DR6: 00000000ffff07f0 DR7: 0000000000000400
> 	PKRU: 55555554
> 	Call Trace:
> 	 <TASK>
> 	 ? show_regs+0x6c/0x80
> 	 ? die_addr+0x37/0xa0
> 	 ? exc_general_protection+0x1c1/0x440
> 	 ? asm_exc_general_protection+0x27/0x30
> 	 ? power_supply_get_property+0x3e/0xe0
> 	 power_supply_hwmon_read+0x50/0xe0
> 	 hwmon_attr_show+0x46/0x170
> 	 dev_attr_show+0x1a/0x70
> 	 sysfs_kf_seq_show+0xaa/0x120
> 	 kernfs_seq_show+0x41/0x60
> 
> Just use the buffer size as argument to fix it.
> 
> Fixes: 226ff2e681d0 ("usb: typec: ucsi: Convert connector specific commands to bitmaps")
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/ucsi/ucsi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index c435c0835744a..69caae84879e9 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -651,7 +651,8 @@ static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient)
>  static int ucsi_get_connector_status(struct ucsi_connector *con, bool conn_ack)
>  {
>  	u64 command = UCSI_GET_CONNECTOR_STATUS | UCSI_CONNECTOR_NUMBER(con->num);
> -	size_t size = min(UCSI_GET_CONNECTOR_STATUS_SIZE, UCSI_MAX_DATA_LENGTH(con->ucsi));
> +	size_t size = min(sizeof(con->status),
> +			  UCSI_MAX_DATA_LENGTH(con->ucsi));
>  	int ret;
>  
>  	ret = ucsi_send_command_common(con->ucsi, command, &con->status, size, conn_ack);
> -- 
> 2.47.0
Todd Brandt Dec. 5, 2024, midnight UTC | #5
Hi all, I opened a bugzilla here about this issue, this commit broke
all our lunar lake machines in boot (5 in total):

https://bugzilla.kernel.org/show_bug.cgi?id=219562

I have tested this patch and it appears to have fixed things. I'm about
to test on all machines with a full run of stress testing of S2idle to
be absolutely sure. But you can add me as tested by.

Tested-By: Brandt, Todd E <todd.e.brandt@linux.intel.com>


On Tue, 2024-12-03 at 12:00 -0800, Lucas De Marchi wrote:
> Similar to commit 65c4c9447bfc ("usb: typec: ucsi: Fix a missing bits
> to
> bytes conversion in ucsi_init()"), there was a missing conversion
> from
> bits to bytes. Here the outcome is worse though: since the value is
> lower than UCSI_MAX_DATA_LENGTH, instead of bailing out with an
> error,
> it writes past the buffer size.
> 
> The error is then seen in other places like below:
> 
>         Oops: general protection fault, probably for non-canonical
> address 0x891e812cd0ed968: 0000 [#1] PREEMPT SMP NOPTI
>         CPU: 3 UID: 110 PID: 906 Comm: prometheus-node Not tainted
> 6.13.0-rc1-xe #1
>         Hardware name: Intel Corporation Lunar Lake Client
> Platform/LNL-M LP5 RVP1, BIOS LNLMFWI1.R00.3222.D84.2410171025
> 10/17/2024
>         RIP: 0010:power_supply_get_property+0x3e/0xe0
>         Code: 85 c0 7e 4f 4c 8b 07 89 f3 49 89 d4 49 8b 48 20 48 85
> c9 74 72 49 8b 70 18 31 d2 31 c0 eb 0b 83 c2 01 48 63 c2 48 39 c8 73
> 5d <3b> 1c 86 75 f0 49 8b 40 28 4c 89 e2 89 de ff d0 0f 1f 00 5b 41
> 5c
>         RSP: 0018:ffffc900017dfa50 EFLAGS: 00010246
>         RAX: 0000000000000000 RBX: 0000000000000011 RCX:
> c963b02c06092008
>         RDX: 0000000000000000 RSI: 0891e812cd0ed968 RDI:
> ffff888121dd6800
>         RBP: ffffc900017dfa68 R08: ffff88810a4024b8 R09:
> 0000000000000000
>         R10: 0000000000000000 R11: 0000000000000000 R12:
> ffffc900017dfa78
>         R13: ffff888121dd6800 R14: ffff888138ad2c00 R15:
> ffff88810c57c528
>         FS:  00007713a2ffd6c0(0000) GS:ffff88846f380000(0000)
> knlGS:0000000000000000
>         CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>         CR2: 000000c0004b1000 CR3: 0000000121ce8003 CR4:
> 0000000000f72ef0
>         DR0: 0000000000000000 DR1: 0000000000000000 DR2:
> 0000000000000000
>         DR3: 0000000000000000 DR6: 00000000ffff07f0 DR7:
> 0000000000000400
>         PKRU: 55555554
>         Call Trace:
>          <TASK>
>          ? show_regs+0x6c/0x80
>          ? die_addr+0x37/0xa0
>          ? exc_general_protection+0x1c1/0x440
>          ? asm_exc_general_protection+0x27/0x30
>          ? power_supply_get_property+0x3e/0xe0
>          power_supply_hwmon_read+0x50/0xe0
>          hwmon_attr_show+0x46/0x170
>          dev_attr_show+0x1a/0x70
>          sysfs_kf_seq_show+0xaa/0x120
>          kernfs_seq_show+0x41/0x60
> 
> Just use the buffer size as argument to fix it.
> 
> Fixes: 226ff2e681d0 ("usb: typec: ucsi: Convert connector specific
> commands to bitmaps")
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/usb/typec/ucsi/ucsi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c
> b/drivers/usb/typec/ucsi/ucsi.c
> index c435c0835744a..69caae84879e9 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -651,7 +651,8 @@ static void ucsi_unregister_altmodes(struct
> ucsi_connector *con, u8 recipient)
>  static int ucsi_get_connector_status(struct ucsi_connector *con,
> bool conn_ack)
>  {
>         u64 command = UCSI_GET_CONNECTOR_STATUS |
> UCSI_CONNECTOR_NUMBER(con->num);
> -       size_t size = min(UCSI_GET_CONNECTOR_STATUS_SIZE,
> UCSI_MAX_DATA_LENGTH(con->ucsi));
> +       size_t size = min(sizeof(con->status),
> +                         UCSI_MAX_DATA_LENGTH(con->ucsi));
>         int ret;
>  
>         ret = ucsi_send_command_common(con->ucsi, command, &con-
> >status, size, conn_ack);
diff mbox series

Patch

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index c435c0835744a..69caae84879e9 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -651,7 +651,8 @@  static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient)
 static int ucsi_get_connector_status(struct ucsi_connector *con, bool conn_ack)
 {
 	u64 command = UCSI_GET_CONNECTOR_STATUS | UCSI_CONNECTOR_NUMBER(con->num);
-	size_t size = min(UCSI_GET_CONNECTOR_STATUS_SIZE, UCSI_MAX_DATA_LENGTH(con->ucsi));
+	size_t size = min(sizeof(con->status),
+			  UCSI_MAX_DATA_LENGTH(con->ucsi));
 	int ret;
 
 	ret = ucsi_send_command_common(con->ucsi, command, &con->status, size, conn_ack);