PMBus memory overflow

PMBus memory overflow

[Matt Corallo]

When adding the PMBus entry for a FSP520-20RAB (actually FSP Twins Pro but it appears to be 
identical hardware with a provided SMBus -> USB adapter and it self-reports as an FSP520-20RAB when 
queried over PMBus using the old pmbus_peek.c script) with `echo pmbus 0x59 > 
/sys/bus/i2c/devices/i2c-3/new_device` I got the following BUG_ON (on Proxmox's 6.8.12-8-pve 
kernel). Its redundant and reports back fine on 0x60 and 0x59 for both of its modules (using 
pmbus_peek.c) and I'd already added 0x60 and it BUG'd adding the second module at 0x59.

Apr 17 15:31:19 rackchill-refresh kernel: detected buffer overflow in memcpy
Apr 17 15:31:19 rackchill-refresh kernel: ------------[ cut here ]------------
Apr 17 15:31:19 rackchill-refresh kernel: kernel BUG at lib/string_helpers.c:1048!
Apr 17 15:31:19 rackchill-refresh kernel: invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
Apr 17 15:31:19 rackchill-refresh kernel: CPU: 18 PID: 387839 Comm: bash Tainted: P           O 
  6.8.12-8-pve #1
Apr 17 15:31:19 rackchill-refresh kernel: Hardware name: Supermicro Super Server/X13SAE-F, BIOS 4.1 
10/01/2024
Apr 17 15:31:19 rackchill-refresh kernel: RIP: 0010:fortify_panic+0x13/0x20
Apr 17 15:31:19 rackchill-refresh kernel: Code: cc 66 0f 1f 44 00 00 90 90 90 90 90 90 90 90 90 90 
90 90 90 90 90 90 55 48 89 fe 48 c7 c7 b0 4e bf 90 48 89 e5 e8 4d f6 9a ff <0f> 0b 66 66 2e 0f 1f 84 
00 00 00 00 00 90 90 90 90 90 90 90 90 90
Apr 17 15:31:19 rackchill-refresh kernel: RSP: 0018:ffffab56ae19f708 EFLAGS: 00010246
Apr 17 15:31:19 rackchill-refresh kernel: RAX: 0000000000000022 RBX: ffffab56ae19f766 RCX: 
0000000000000000
Apr 17 15:31:19 rackchill-refresh kernel: RDX: 0000000000000000 RSI: 0000000000000000 RDI: 
0000000000000000
Apr 17 15:31:19 rackchill-refresh kernel: RBP: ffffab56ae19f708 R08: 0000000000000000 R09: 
0000000000000000
Apr 17 15:31:19 rackchill-refresh kernel: R10: 0000000000000000 R11: 0000000000000000 R12: 
ffff8a76f8c98428
Apr 17 15:31:19 rackchill-refresh kernel: R13: ffff8a7481989400 R14: 000000000000009b R15: 
ffff8a8f6fe1a428
Apr 17 15:31:19 rackchill-refresh kernel: FS:  00007f3e871a1740(0000) GS:ffff8a937fd00000(0000) 
knlGS:0000000000000000
Apr 17 15:31:19 rackchill-refresh kernel: CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
Apr 17 15:31:19 rackchill-refresh kernel: CR2: 00007ff6328e0000 CR3: 00000011f6ba0002 CR4: 
0000000000f72ef0
Apr 17 15:31:19 rackchill-refresh kernel: PKRU: 55555554
Apr 17 15:31:19 rackchill-refresh kernel: Call Trace:
Apr 17 15:31:19 rackchill-refresh kernel:  <TASK>
Apr 17 15:31:19 rackchill-refresh kernel:  ? show_regs+0x6d/0x80
Apr 17 15:31:19 rackchill-refresh kernel:  ? die+0x37/0xa0
Apr 17 15:31:19 rackchill-refresh kernel:  ? do_trap+0xd4/0xf0
Apr 17 15:31:19 rackchill-refresh kernel:  ? do_error_trap+0x71/0xb0
Apr 17 15:31:19 rackchill-refresh kernel:  ? fortify_panic+0x13/0x20
Apr 17 15:31:19 rackchill-refresh kernel:  ? exc_invalid_op+0x52/0x80
Apr 17 15:31:19 rackchill-refresh kernel:  ? fortify_panic+0x13/0x20
Apr 17 15:31:19 rackchill-refresh kernel:  ? asm_exc_invalid_op+0x1b/0x20
Apr 17 15:31:19 rackchill-refresh kernel:  ? fortify_panic+0x13/0x20
Apr 17 15:31:19 rackchill-refresh kernel:  ? fortify_panic+0x13/0x20
Apr 17 15:31:19 rackchill-refresh kernel:  i2c_smbus_read_block_data+0x116/0x120
Apr 17 15:31:19 rackchill-refresh kernel:  pmbus_check_block_register.constprop.0+0x77/0x100 
[pmbus_core]
Apr 17 15:31:19 rackchill-refresh kernel:  pmbus_do_probe+0x11a5/0x1e00 [pmbus_core]
Apr 17 15:31:19 rackchill-refresh kernel:  ? __kmalloc_node_track_caller+0x1c9/0x430
Apr 17 15:31:19 rackchill-refresh kernel:  ? devres_add+0x6f/0xf0
Apr 17 15:31:19 rackchill-refresh kernel:  pmbus_probe+0x74/0xb0 [pmbus]
Apr 17 15:31:19 rackchill-refresh kernel:  i2c_device_probe+0x13b/0x300
Apr 17 15:31:19 rackchill-refresh kernel:  really_probe+0x1c9/0x430
Apr 17 15:31:19 rackchill-refresh kernel:  __driver_probe_device+0x8c/0x190
Apr 17 15:31:19 rackchill-refresh kernel:  driver_probe_device+0x24/0xd0
Apr 17 15:31:19 rackchill-refresh kernel:  __device_attach_driver+0xcd/0x170
Apr 17 15:31:19 rackchill-refresh kernel:  ? __pfx___device_attach_driver+0x10/0x10
Apr 17 15:31:19 rackchill-refresh kernel:  bus_for_each_drv+0x94/0xf0
Apr 17 15:31:19 rackchill-refresh kernel:  __device_attach+0xb6/0x1d0
Apr 17 15:31:19 rackchill-refresh kernel:  device_initial_probe+0x13/0x20
Apr 17 15:31:19 rackchill-refresh kernel:  bus_probe_device+0x9f/0xb0
Apr 17 15:31:19 rackchill-refresh kernel:  device_add+0x6d7/0x8f0
Apr 17 15:31:19 rackchill-refresh kernel:  ? hrtimer_init+0x27/0x90
Apr 17 15:31:19 rackchill-refresh kernel:  device_register+0x1a/0x30
Apr 17 15:31:19 rackchill-refresh kernel:  i2c_new_client_device+0x1fb/0x3f0
Apr 17 15:31:19 rackchill-refresh kernel:  new_device_store+0x113/0x290
Apr 17 15:31:19 rackchill-refresh kernel:  dev_attr_store+0x14/0x40
Apr 17 15:31:19 rackchill-refresh kernel:  sysfs_kf_write+0x3b/0x60
Apr 17 15:31:19 rackchill-refresh kernel:  kernfs_fop_write_iter+0x130/0x210
Apr 17 15:31:19 rackchill-refresh kernel:  vfs_write+0x2a5/0x480
Apr 17 15:31:19 rackchill-refresh kernel:  ksys_write+0x73/0x100
Apr 17 15:31:19 rackchill-refresh kernel:  __x64_sys_write+0x19/0x30
Apr 17 15:31:19 rackchill-refresh kernel:  x64_sys_call+0x200f/0x2480
Apr 17 15:31:19 rackchill-refresh kernel:  do_syscall_64+0x81/0x170
Apr 17 15:31:19 rackchill-refresh kernel:  ? __handle_mm_fault+0xba9/0xf70
Apr 17 15:31:19 rackchill-refresh kernel:  ? __count_memcg_events+0x6f/0xe0
Apr 17 15:31:19 rackchill-refresh kernel:  ? count_memcg_events.constprop.0+0x2a/0x50
Apr 17 15:31:19 rackchill-refresh kernel:  ? handle_mm_fault+0xad/0x380
Apr 17 15:31:19 rackchill-refresh kernel:  ? do_user_addr_fault+0x33e/0x660
Apr 17 15:31:19 rackchill-refresh kernel:  ? irqentry_exit_to_user_mode+0x7b/0x260
Apr 17 15:31:19 rackchill-refresh kernel:  ? irqentry_exit+0x43/0x50
Apr 17 15:31:19 rackchill-refresh kernel:  ? exc_page_fault+0x94/0x1b0
Apr 17 15:31:19 rackchill-refresh kernel:  entry_SYSCALL_64_after_hwframe+0x78/0x80
Apr 17 15:31:19 rackchill-refresh kernel: RIP: 0033:0x7f3e8729c300
Apr 17 15:31:19 rackchill-refresh kernel: Code: 40 00 48 8b 15 01 9b 0d 00 f7 d8 64 89 02 48 c7 c0 
ff ff ff ff eb b7 0f 1f 00 80 3d e1 22 0e 00 00 74 17 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 58 
c3 0f 1f 80 00 00 00 00 48 83 ec 28 48 89
Apr 17 15:31:19 rackchill-refresh kernel: RSP: 002b:00007ffcd6171828 EFLAGS: 00000202 ORIG_RAX: 
0000000000000001
Apr 17 15:31:19 rackchill-refresh kernel: RAX: ffffffffffffffda RBX: 000000000000000b RCX: 
00007f3e8729c300
Apr 17 15:31:19 rackchill-refresh kernel: RDX: 000000000000000b RSI: 00005a5e6f9bb6f0 RDI: 
0000000000000001
Apr 17 15:31:19 rackchill-refresh kernel: RBP: 00005a5e6f9bb6f0 R08: 0000000000000007 R09: 
0000000000000073
Apr 17 15:31:19 rackchill-refresh kernel: R10: 0000000000000000 R11: 0000000000000202 R12: 
000000000000000b
Apr 17 15:31:19 rackchill-refresh kernel: R13: 00007f3e87377760 R14: 000000000000000b R15: 
00007f3e873729e0
Apr 17 15:31:19 rackchill-refresh kernel:  </TASK>
Apr 17 15:31:19 rackchill-refresh kernel: Modules linked in: pmbus pmbus_core tcp_diag inet_diag 
ebt_arp xt_mac veth rbd ceph libceph cmac nls_utf8 cifs cifs_arc4 nls_ucs2_utils rdma_cm iw_cm ib_cm 
cifs_md4 netfs ebtable_filter ebtables ip6table_raw ip6t_REJECT nf_reject_ipv6 ip6table_filter 
ip6_tables iptable_raw ipt_REJECT nf_reject_ipv4 xt_mark xt_set xt_physdev xt_addrtype xt_comment 
xt_multiport xt_conntrack nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 xt_tcpudp iptable_filter 
ip_set_hash_net ip_set sctp ip6_udp_tunnel udp_tunnel nf_tables nvme_fabrics nvme_keyring bonding 
sunrpc softdog nfnetlink_log binfmt_misc nfnetlink intel_rapl_msr intel_rapl_common 
intel_uncore_frequency intel_uncore_frequency_common snd_sof_pci_intel_tgl snd_sof_intel_hda_common 
soundwire_intel snd_sof_intel_hda_mlink soundwire_cadence snd_sof_intel_hda x86_pkg_temp_thermal 
snd_sof_pci intel_powerclamp snd_sof_xtensa_dsp snd_sof snd_sof_utils snd_soc_hdac_hda coretemp 
snd_hda_ext_core snd_soc_acpi_intel_match snd_soc_acpi soundwire_generic_allocation soundwire_bus
Apr 17 15:31:19 rackchill-refresh kernel:  kvm_intel snd_soc_core snd_hda_codec_realtek snd_compress 
ac97_bus snd_hda_codec_generic snd_pcm_dmaengine snd_hda_intel kvm snd_intel_dspcfg 
snd_intel_sdw_acpi snd_hda_codec snd_hda_core ipmi_ssif irqbypass snd_hwdep mei_hdcp mei_pxp 
cmdlinepart snd_pcm spi_nor snd_timer rapl ast snd acpi_ipmi intel_cstate ucsi_acpi mtd wmi_bmof 
i2c_algo_bit soundcore mei_me pcspkr intel_pmc_core typec_ucsi ipmi_si mei ipmi_devintf intel_vsec 
typec pmt_telemetry ipmi_msghandler pmt_class acpi_pad acpi_tad joydev input_leds mac_hid zfs(PO) 
spl(O) vhost_net vhost vhost_iotlb tap nct6775 nct6775_core hwmon_vid efi_pstore dmi_sysfs ip_tables 
x_tables autofs4 btrfs blake2b_generic xor raid6_pq libcrc32c dm_crypt mlx5_ib ib_uverbs macsec 
ib_core rndis_host cdc_ether usbnet mii usbmouse hid_cp2112 hid_generic usbhid hid crct10dif_pclmul 
mlx5_core crc32_pclmul polyval_clmulni nvme xhci_pci polyval_generic ghash_clmulni_intel 
xhci_pci_renesas sha256_ssse3 sha1_ssse3 e1000e intel_lpss_pci mlxfw nvme_core i2c_i801 ahci igc psample
Apr 17 15:31:19 rackchill-refresh kernel:  intel_lpss spi_intel_pci xhci_hcd spi_intel i2c_smbus tls 
idma64 nvme_auth libahci pci_hyperv_intf video pinctrl_alderlake wmi aesni_intel crypto_simd cryptd
Apr 17 15:31:19 rackchill-refresh kernel: ---[ end trace 0000000000000000 ]---
Apr 17 15:31:19 rackchill-refresh kernel: RIP: 0010:fortify_panic+0x13/0x20
Apr 17 15:31:19 rackchill-refresh kernel: Code: cc 66 0f 1f 44 00 00 90 90 90 90 90 90 90 90 90 90 
90 90 90 90 90 90 55 48 89 fe 48 c7 c7 b0 4e bf 90 48 89 e5 e8 4d f6 9a ff <0f> 0b 66 66 2e 0f 1f 84 
00 00 00 00 00 90 90 90 90 90 90 90 90 90
Apr 17 15:31:19 rackchill-refresh kernel: RSP: 0018:ffffab56ae19f708 EFLAGS: 00010246
Apr 17 15:31:19 rackchill-refresh kernel: RAX: 0000000000000022 RBX: ffffab56ae19f766 RCX: 
0000000000000000
Apr 17 15:31:19 rackchill-refresh kernel: RDX: 0000000000000000 RSI: 0000000000000000 RDI: 
0000000000000000
Apr 17 15:31:19 rackchill-refresh kernel: RBP: ffffab56ae19f708 R08: 0000000000000000 R09: 
0000000000000000
Apr 17 15:31:19 rackchill-refresh kernel: R10: 0000000000000000 R11: 0000000000000000 R12: 
ffff8a76f8c98428
Apr 17 15:31:19 rackchill-refresh kernel: R13: ffff8a7481989400 R14: 000000000000009b R15: 
ffff8a8f6fe1a428
Apr 17 15:31:19 rackchill-refresh kernel: FS:  00007f3e871a1740(0000) GS:ffff8a937fd00000(0000) 
knlGS:0000000000000000
Apr 17 15:31:19 rackchill-refresh kernel: CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
Apr 17 15:31:19 rackchill-refresh kernel: CR2: 00007ff6328e0000 CR3: 00000011f6ba0002 CR4: 
0000000000f72ef0
Apr 17 15:31:19 rackchill-refresh kernel: PKRU: 55555554

Re: PMBus memory overflow

[Guenter Roeck]

On Thu, Apr 17, 2025 at 11:39:14AM -0400, Matt Corallo wrote:
> When adding the PMBus entry for a FSP520-20RAB (actually FSP Twins Pro but
> it appears to be identical hardware with a provided SMBus -> USB adapter and
> it self-reports as an FSP520-20RAB when queried over PMBus using the old
> pmbus_peek.c script) with `echo pmbus 0x59 >
> /sys/bus/i2c/devices/i2c-3/new_device` I got the following BUG_ON (on
> Proxmox's 6.8.12-8-pve kernel). Its redundant and reports back fine on 0x60
> and 0x59 for both of its modules (using pmbus_peek.c) and I'd already added
> 0x60 and it BUG'd adding the second module at 0x59.
> 
...
> Apr 17 15:31:19 rackchill-refresh kernel:  i2c_smbus_read_block_data+0x116/0x120
> Apr 17 15:31:19 rackchill-refresh kernel:
> pmbus_check_block_register.constprop.0+0x77/0x100 [pmbus_core]

Interesting. That function reads into a buffer which is larger than the
largest valid PMBus transaction. I can only imagine that the controller
returns invalid data in that transaction, where the first returned byte
is not the length of the transfer but something else.
i2c_smbus_read_block_data() does an unconditional
	memcpy(values, &data.block[1], data.block[0]);
which of course will go haywire if data.block[0] (or in other words the
first data byte returned from the device) exceeds the size of the
data buffer. Do you happen to have a datasheet ?

Guenter

Re: PMBus memory overflow

[Matt Corallo]

On 4/17/25 2:00 PM, Guenter Roeck wrote:
> On Thu, Apr 17, 2025 at 11:39:14AM -0400, Matt Corallo wrote:
>> When adding the PMBus entry for a FSP520-20RAB (actually FSP Twins Pro but
>> it appears to be identical hardware with a provided SMBus -> USB adapter and
>> it self-reports as an FSP520-20RAB when queried over PMBus using the old
>> pmbus_peek.c script) with `echo pmbus 0x59 >
>> /sys/bus/i2c/devices/i2c-3/new_device` I got the following BUG_ON (on
>> Proxmox's 6.8.12-8-pve kernel). Its redundant and reports back fine on 0x60
>> and 0x59 for both of its modules (using pmbus_peek.c) and I'd already added
>> 0x60 and it BUG'd adding the second module at 0x59.
>>
> ...
>> Apr 17 15:31:19 rackchill-refresh kernel:  i2c_smbus_read_block_data+0x116/0x120
>> Apr 17 15:31:19 rackchill-refresh kernel:
>> pmbus_check_block_register.constprop.0+0x77/0x100 [pmbus_core]
> 
> Interesting. That function reads into a buffer which is larger than the
> largest valid PMBus transaction. I can only imagine that the controller
> returns invalid data in that transaction, where the first returned byte
> is not the length of the transfer but something else.
> i2c_smbus_read_block_data() does an unconditional
> 	memcpy(values, &data.block[1], data.block[0]);
> which of course will go haywire if data.block[0] (or in other words the
> first data byte returned from the device) exceeds the size of the
> data buffer. Do you happen to have a datasheet ?

I do not, sadly (though FSP support has been rumored to help out at least marginally, though they 
haven't been useful for me). Interestingly the (I guess ancient now) pmbus_peek.c script has no 
issues reading from it (added a quick print on the -E2BIG line and it didn't get hit). pmbuss_peek.c 
says the following:

root@rackchill-refresh:~# ./a.out -b /dev/i2c-3 -s 0x59
PMBus slave on /dev/i2c-3, address 0x59

Inventory Data:
   Manufacturer:		FSP-GROUP
   Model:		FSP520-20RAB
   Revision:		(null)
   Built on:		
   Serial:		
   IC Device:		PIC24FJ32GA004

PMBus revisions (0x22):	part I, ver 1.1; part II, ver 1.2
Capabilities (0x90):	PEC, SMBALERT#, 100 KHz

Status 0000:

Attribute Values:
   page                  00: (BITMAP)
   operation             00: (BITMAP)
   on_off_config         15: (BITMAP)
   vout_mode             17: (BITMAP)
   fan_command_1         0000: 0
   ein                   ba4ba00938ba: 5.26211e+06
   eout                  1e56a12038ba: 5.29753e+06
   vin                   f1e8: 122 Volts
   iin                   c09c: 0.609375 Amperes
   vout                  1869: 12.2051 Volts
   iout                  caad: 5.35156 Amperes
   temperature_1         001c: 28 degrees Celsius
   temperature_2         0024: 36 degrees Celsius
   fan_speed_1           1a1f: 4344
   frequency             f0ec: 59
   pout                  f108: 66 Watts
   pin                   f128: 74 Watts
   mfr_vout_min          0000: 0 Volts
   mfr_iout_max          0000: 0 Amperes

root@rackchill-refresh:~# ./a.out -b /dev/i2c-3 -s 0x58
PMBus slave on /dev/i2c-3, address 0x58

Inventory Data:
   Manufacturer:		FSP-GROUP
   Model:		FSP520-20RAB
   Revision:		(null)
   Built on:		
   Serial:		
   IC Device:		PIC24FJ32GA004

PMBus revisions (0x22):	part I, ver 1.1; part II, ver 1.2
Capabilities (0x90):	PEC, SMBALERT#, 100 KHz

Status 0000:

Attribute Values:
   page                  00: (BITMAP)
   operation             00: (BITMAP)
   on_off_config         15: (BITMAP)
   vout_mode             17: (BITMAP)
   fan_command_1         0000: 0
   ein                   0574e89f8070: 7.63164e+06
   eout                  613f88b78070: 4.47254e+06
   vin                   f1e8: 122 Volts
   iin                   c039: 0.222656 Amperes
   vout                  1871: 12.2207 Volts
   iout                  c11e: 1.11719 Amperes
   temperature_1         001b: 27 degrees Celsius
   temperature_2         0024: 36 degrees Celsius
   fan_speed_1           13e5: 3988
   frequency             f0ec: 59
   pout                  f038: 14 Watts
   pin                   f050: 20 Watts
   mfr_vout_min          0000: 0 Volts
   mfr_iout_max          0000: 0 Amperes

Re: PMBus memory overflow

[Guenter Roeck]

On 4/17/25 11:14, Matt Corallo wrote:
> 
> 
> On 4/17/25 2:00 PM, Guenter Roeck wrote:
>> On Thu, Apr 17, 2025 at 11:39:14AM -0400, Matt Corallo wrote:
>>> When adding the PMBus entry for a FSP520-20RAB (actually FSP Twins Pro but
>>> it appears to be identical hardware with a provided SMBus -> USB adapter and
>>> it self-reports as an FSP520-20RAB when queried over PMBus using the old
>>> pmbus_peek.c script) with `echo pmbus 0x59 >
>>> /sys/bus/i2c/devices/i2c-3/new_device` I got the following BUG_ON (on
>>> Proxmox's 6.8.12-8-pve kernel). Its redundant and reports back fine on 0x60
>>> and 0x59 for both of its modules (using pmbus_peek.c) and I'd already added
>>> 0x60 and it BUG'd adding the second module at 0x59.
>>>
>> ...
>>> Apr 17 15:31:19 rackchill-refresh kernel:  i2c_smbus_read_block_data+0x116/0x120
>>> Apr 17 15:31:19 rackchill-refresh kernel:
>>> pmbus_check_block_register.constprop.0+0x77/0x100 [pmbus_core]
>>
>> Interesting. That function reads into a buffer which is larger than the
>> largest valid PMBus transaction. I can only imagine that the controller
>> returns invalid data in that transaction, where the first returned byte
>> is not the length of the transfer but something else.
>> i2c_smbus_read_block_data() does an unconditional
>>     memcpy(values, &data.block[1], data.block[0]);
>> which of course will go haywire if data.block[0] (or in other words the
>> first data byte returned from the device) exceeds the size of the
>> data buffer. Do you happen to have a datasheet ?
> 
> I do not, sadly (though FSP support has been rumored to help out at least marginally, though they haven't been useful for me). Interestingly the (I guess ancient now) pmbus_peek.c script has no issues reading from it (added a quick print on the -E2BIG line and it didn't get hit). pmbuss_peek.c says the following:
> 
> root@rackchill-refresh:~# ./a.out -b /dev/i2c-3 -s 0x59
> PMBus slave on /dev/i2c-3, address 0x59
> 

pmbus_peek supports reading up to 255 bytes into the receive buffer.
Anything above 32 bytes violates the SMBus specification. I found that
the I2C controller driver should block that. If it doesn't, all kinds
of chips could trigger this problem. Do you know which I2C controller
is used by that system ? You mentioned an SMBus - USB adapter. The drivers
for the adapters I am aware of (Diolan, Devantech) do validate the return
length, so I assume you use something else or maybe an out-of-tree driver.

Please let me know the adapter you use. If the driver is not upstream,
a pointer to the driver source would also be helpful.

Thanks,
Guenter

Re: PMBus memory overflow

[Matt Corallo]

On 4/17/25 9:21 PM, Guenter Roeck wrote:
> On 4/17/25 11:14, Matt Corallo wrote:
>> I do not, sadly (though FSP support has been rumored to help out at least marginally, though they 
>> haven't been useful for me). Interestingly the (I guess ancient now) pmbus_peek.c script has no 
>> issues reading from it (added a quick print on the -E2BIG line and it didn't get hit). 
>> pmbuss_peek.c says the following:
>>
>> root@rackchill-refresh:~# ./a.out -b /dev/i2c-3 -s 0x59
>> PMBus slave on /dev/i2c-3, address 0x59
>>
> 
> pmbus_peek supports reading up to 255 bytes into the receive buffer.

Hmm, I'm using this version, which on L622 checks for a length > 32 (and doesn't get hit in the -s 
command) - https://github.com/jktjkt/pmbus_peek/blob/master/pmbus_peek.c#L622

> Anything above 32 bytes violates the SMBus specification. I found that
> the I2C controller driver should block that. If it doesn't, all kinds
> of chips could trigger this problem. Do you know which I2C controller
> is used by that system ? You mentioned an SMBus - USB adapter. The drivers
> for the adapters I am aware of (Diolan, Devantech) do validate the return
> length, so I assume you use something else or maybe an out-of-tree driver.

The driver appears to be in-tree (or at least was auto-loaded by the Proxmox kernel, which is mostly 
just the Ubuntu kernel). Its a 10c4:ea90 Silicon Labs CP2112 HID I2C Bridge.

Still, more generally, presumably it shouldn't be the case that a defective USB device can cause the 
kernel to memcpy past a buffer? I guess for hardened kernels it gets caught (though once the process 
gets killed by the hardening the system is still somewhat shaky, reboots dont work etc), but is 
there a build option that would turn this into a security vuln?

Matt

Re: PMBus memory overflow

[Guenter Roeck]

On 4/18/25 14:03, Matt Corallo wrote:
> 
> 
> On 4/17/25 9:21 PM, Guenter Roeck wrote:
>> On 4/17/25 11:14, Matt Corallo wrote:
>>> I do not, sadly (though FSP support has been rumored to help out at least marginally, though they haven't been useful for me). Interestingly the (I guess ancient now) pmbus_peek.c script has no issues reading from it (added a quick print on the -E2BIG line and it didn't get hit). pmbuss_peek.c says the following:
>>>
>>> root@rackchill-refresh:~# ./a.out -b /dev/i2c-3 -s 0x59
>>> PMBus slave on /dev/i2c-3, address 0x59
>>>
>>
>> pmbus_peek supports reading up to 255 bytes into the receive buffer.
> 
> Hmm, I'm using this version, which on L622 checks for a length > 32 (and doesn't get hit in the -s command) - https://github.com/jktjkt/pmbus_peek/blob/master/pmbus_peek.c#L622
> 

Sorry, I am only guessing here. The code in pmbus_peek.c is a bit odd, though:
It does validate the first byte, but then it jumps to try_i2c: and tries again,
this time reading up to 255 bytes. I am not really sure if that ends up reporting
an error or not. The comment "NOTE:  this probably won't be visible" doesn't
really improve my confidence that it will report the problem.

>> Anything above 32 bytes violates the SMBus specification. I found that
>> the I2C controller driver should block that. If it doesn't, all kinds
>> of chips could trigger this problem. Do you know which I2C controller
>> is used by that system ? You mentioned an SMBus - USB adapter. The drivers
>> for the adapters I am aware of (Diolan, Devantech) do validate the return
>> length, so I assume you use something else or maybe an out-of-tree driver.
> 
> The driver appears to be in-tree (or at least was auto-loaded by the Proxmox kernel, which is mostly just the Ubuntu kernel). Its a 10c4:ea90 Silicon Labs CP2112 HID I2C Bridge.
> 

So that is drivers/hid/hid-cp2112.c. Unless I am missing something, that driver
does not validate the contents of the first returned data byte when executing
an I2C_SMBUS_BLOCK_DATA read command.

> Still, more generally, presumably it shouldn't be the case that a defective USB device can cause the kernel to memcpy past a buffer? I guess for hardened kernels it gets caught (though once the process gets killed by the hardening the system is still somewhat shaky, reboots dont work etc), but is there a build option that would turn this into a security vuln?
> 

Exactly my point. A defective USB device should most definitely _not_ cause the
kernel to memcpy past the end of a buffer.

Of course, question is if i2c_smbus_read_block_data() should blindly copy more
than 32 bytes just because the first data byte returned from the device says so,
but that is a question to ask the i2c maintainer after we figure out what is
going on.

If you are up to compiling code, it would be great if you could add some debug logs
into pmbus_peek.c and/or into the hid-cp2112.c driver to see what is actually
returned from the chip.

Thanks,
Guenter

Re: PMBus memory overflow

[Matt Corallo]

On 4/18/25 6:30 PM, Guenter Roeck wrote:
> On 4/18/25 14:03, Matt Corallo wrote:
>>
>>
>> On 4/17/25 9:21 PM, Guenter Roeck wrote:
>>> On 4/17/25 11:14, Matt Corallo wrote:
>>>> I do not, sadly (though FSP support has been rumored to help out at least marginally, though 
>>>> they haven't been useful for me). Interestingly the (I guess ancient now) pmbus_peek.c script 
>>>> has no issues reading from it (added a quick print on the -E2BIG line and it didn't get hit). 
>>>> pmbuss_peek.c says the following:
>>>>
>>>> root@rackchill-refresh:~# ./a.out -b /dev/i2c-3 -s 0x59
>>>> PMBus slave on /dev/i2c-3, address 0x59
>>>>
>>>
>>> pmbus_peek supports reading up to 255 bytes into the receive buffer.
>>
>> Hmm, I'm using this version, which on L622 checks for a length > 32 (and doesn't get hit in the -s 
>> command) - https://github.com/jktjkt/pmbus_peek/blob/master/pmbus_peek.c#L622
>>
> 
> Sorry, I am only guessing here. The code in pmbus_peek.c is a bit odd, though:
> It does validate the first byte, but then it jumps to try_i2c: and tries again,
> this time reading up to 255 bytes. I am not really sure if that ends up reporting
> an error or not. The comment "NOTE:  this probably won't be visible" doesn't
> really improve my confidence that it will report the problem.

Sure, that's why I `printf("BAD LEN\n");`'d there but didn't see it get hit.


> If you are up to compiling code, it would be great if you could add some debug logs
> into pmbus_peek.c and/or into the hid-cp2112.c driver to see what is actually
> returned from the chip.

Sure, of course. Obviously easier to do in pmbus_peek if you have any suggested logs you want. The 
device caps output is (and still never hit my BAD LEN printf):

./a.out -b /dev/i2c-3 -l -v 0x58
PMBus slave on /dev/i2c-3, address 0x58

Inventory Data:
   Manufacturer:		FSP-GROUP
   Model:		FSP520-20RAB
   Revision:		(null)
   Built on:		
   Serial:		
   IC Device:		PIC24FJ32GA004

PMBus revisions (0x22):	part I, ver 1.1; part II, ver 1.2
Capabilities (0x90):	PEC, SMBALERT#, 100 KHz

Supported Commands:
   00 page                      rw u8 (bitmask)
   01 operation                 rw u8 (bitmask)
   02 on_off_config             rw u8 (bitmask)
   03 clear_fault                w nodata
   05 page_plus_read             w block
   19 capability                r  u8 (bitmask)
   1a query                     rw process_call
   1b smbalert_mask             rw block
   20 vout_mode                 r  u8 (bitmask)
   30 coefficients              r  process_call
   3b fan_command_1             rw s16 (LINEAR)
   79 status_word               r  u16 (bitmask)
   7a status_vout               r  u8 (bitmask)
   7b status_iout               r  u8 (bitmask)
   7c status_input              r  u8 (bitmask)
   7d status_temperature        r  u8 (bitmask)
   81 status_fans_1_2           rw u8 (bitmask)
   86 read_ein                  r  block(6), Energy counter (DIRECT)
   87 read_eout                 r  block(6), Energy counter (DIRECT)
   88 read_vin                  r  s16 (LINEAR), Volts
   89 read_iin                  r  s16 (LINEAR), Amperes
   8b read_vout                 r  x16 (VOUT_MODE), Volts
   8c read_iout                 r  s16 (LINEAR), Amperes
   8d read_temperature_1        r  s16 (LINEAR), degrees Celsius
   8e read_temperature_2        r  s16 (LINEAR), degrees Celsius
   90 read_fan_speed_1          r  s16 (LINEAR)
   95 read_frequency            r  s16 (LINEAR)
   96 read_pout                 r  s16 (LINEAR), Watts
   97 read_pin                  r  s16 (LINEAR), Watts
   98 pmbus_revision            r  u8 (bitmask)
   99 mfr_id                    r  block, ISO 8859/1 string
   9a mfr_model                 r  block, ISO 8859/1 string
   9d mfr_date                  r  block, ISO 8859/1 string
   9e mfr_serial                r  block, ISO 8859/1 string
   a4 mfr_vout_min              r  s16 (LINEAR), Volts
   a5 mfr_iout_max              r  s16 (LINEAR), Amperes
   ad ic_device_id              r  block, ISO 8859/1 string
   d1 mfr_specific_01           r  (UNKNOWN call syntax)
   d2 mfr_specific_02           r  (UNKNOWN call syntax)
   d4 mfr_specific_04           r  (UNKNOWN call syntax)
   d5 mfr_specific_05           r  (UNKNOWN call syntax)
   d7 mfr_specific_07           rw (UNKNOWN call syntax)
   d8 mfr_specific_08           r  (UNKNOWN call syntax)
   d9 mfr_specific_09           r  (UNKNOWN call syntax)
   da mfr_specific_10           r  (UNKNOWN call syntax)
   db mfr_specific_11           r  (UNKNOWN call syntax)
   de mfr_specific_14           r  (UNKNOWN call syntax)
   e0 mfr_specific_16           r  (UNKNOWN call syntax)
   e1 mfr_specific_17           rw (UNKNOWN call syntax)
   e2 mfr_specific_18           rw (UNKNOWN call syntax)
   e3 mfr_specific_19           rw (UNKNOWN call syntax)
   e5 mfr_specific_21            w (UNKNOWN call syntax)
   f0 mfr_specific_32           rw (UNKNOWN call syntax)

Re: PMBus memory overflow

[Guenter Roeck]

On 4/19/25 10:53, Matt Corallo wrote:
> 
> 
> On 4/18/25 6:30 PM, Guenter Roeck wrote:
>> On 4/18/25 14:03, Matt Corallo wrote:
>>>
>>>
>>> On 4/17/25 9:21 PM, Guenter Roeck wrote:
>>>> On 4/17/25 11:14, Matt Corallo wrote:
>>>>> I do not, sadly (though FSP support has been rumored to help out at least marginally, though they haven't been useful for me). Interestingly the (I guess ancient now) pmbus_peek.c script has no issues reading from it (added a quick print on the -E2BIG line and it didn't get hit). pmbuss_peek.c says the following:
>>>>>
>>>>> root@rackchill-refresh:~# ./a.out -b /dev/i2c-3 -s 0x59
>>>>> PMBus slave on /dev/i2c-3, address 0x59
>>>>>
>>>>
>>>> pmbus_peek supports reading up to 255 bytes into the receive buffer.
>>>
>>> Hmm, I'm using this version, which on L622 checks for a length > 32 (and doesn't get hit in the -s command) - https://github.com/jktjkt/pmbus_peek/blob/master/pmbus_peek.c#L622
>>>
>>
>> Sorry, I am only guessing here. The code in pmbus_peek.c is a bit odd, though:
>> It does validate the first byte, but then it jumps to try_i2c: and tries again,
>> this time reading up to 255 bytes. I am not really sure if that ends up reporting
>> an error or not. The comment "NOTE:  this probably won't be visible" doesn't
>> really improve my confidence that it will report the problem.
> 
> Sure, that's why I `printf("BAD LEN\n");`'d there but didn't see it get hit.
> 
> 
>> If you are up to compiling code, it would be great if you could add some debug logs
>> into pmbus_peek.c and/or into the hid-cp2112.c driver to see what is actually
>> returned from the chip.
> 
> Sure, of course. Obviously easier to do in pmbus_peek if you have any suggested logs you want. The device caps output is (and still never hit my BAD LEN printf):
> 
> ./a.out -b /dev/i2c-3 -l -v 0x58
> PMBus slave on /dev/i2c-3, address 0x58
> 
> Inventory Data:
>    Manufacturer:        FSP-GROUP
>    Model:        FSP520-20RAB
>    Revision:        (null)
>    Built on:
>    Serial:
>    IC Device:        PIC24FJ32GA004
> 
> PMBus revisions (0x22):    part I, ver 1.1; part II, ver 1.2
> Capabilities (0x90):    PEC, SMBALERT#, 100 KHz
> 
> Supported Commands:
>    00 page                      rw u8 (bitmask)
>    01 operation                 rw u8 (bitmask)
>    02 on_off_config             rw u8 (bitmask)
>    03 clear_fault                w nodata
>    05 page_plus_read             w block
>    19 capability                r  u8 (bitmask)
>    1a query                     rw process_call

pmbus_peek uses the query command to determine if a command
is supported or not. It doesn't try to read other commands.

>    1b smbalert_mask             rw block
>    20 vout_mode                 r  u8 (bitmask)
>    30 coefficients              r  process_call
>    3b fan_command_1             rw s16 (LINEAR)
>    79 status_word               r  u16 (bitmask)
>    7a status_vout               r  u8 (bitmask)
>    7b status_iout               r  u8 (bitmask)
>    7c status_input              r  u8 (bitmask)
>    7d status_temperature        r  u8 (bitmask)
>    81 status_fans_1_2           rw u8 (bitmask)
>    86 read_ein                  r  block(6), Energy counter (DIRECT)
>    87 read_eout                 r  block(6), Energy counter (DIRECT)
>    88 read_vin                  r  s16 (LINEAR), Volts
>    89 read_iin                  r  s16 (LINEAR), Amperes
>    8b read_vout                 r  x16 (VOUT_MODE), Volts
>    8c read_iout                 r  s16 (LINEAR), Amperes
>    8d read_temperature_1        r  s16 (LINEAR), degrees Celsius
>    8e read_temperature_2        r  s16 (LINEAR), degrees Celsius
>    90 read_fan_speed_1          r  s16 (LINEAR)
>    95 read_frequency            r  s16 (LINEAR)
>    96 read_pout                 r  s16 (LINEAR), Watts
>    97 read_pin                  r  s16 (LINEAR), Watts
>    98 pmbus_revision            r  u8 (bitmask)
>    99 mfr_id                    r  block, ISO 8859/1 string
>    9a mfr_model                 r  block, ISO 8859/1 string
>    9d mfr_date                  r  block, ISO 8859/1 string
>    9e mfr_serial                r  block, ISO 8859/1 string

That also means it does not try to read mfr_revision or mfr_location
since that is not supported. The PMBus driver in the kernel does
try to read those (it doesn't use the query command to determine
if a command is supported or not).

My suspicion is that the power supply returns something (but not
valid SMBus block data) when reading those commands. If modifying
the kernel to find out is not easy, another option might be to
enable smbus tracing. Would it be possible to do that ?
Another option might be to modify the pmbus_peek command to read
those registers, or maybe even better to use i2cget to read the
data directly.

Somewhat unrelated to this, it might be time to revisit using the
QUERY command to determine what is supported. If only I had an endless
amount of time ... oh well. When I wrote the kernel driver, none of
the chips I had available at the time supported that command :-(.

Thanks,
Guenter

Re: PMBus memory overflow

[Matt Corallo]

On 4/19/25 3:05 PM, Guenter Roeck wrote:
> On 4/19/25 10:53, Matt Corallo wrote:
>>
>> Sure, of course. Obviously easier to do in pmbus_peek if you have any suggested logs you want. The 
>> device caps output is (and still never hit my BAD LEN printf):
>>
>> ./a.out -b /dev/i2c-3 -l -v 0x58
>> PMBus slave on /dev/i2c-3, address 0x58
>>
>> Inventory Data:
>>    Manufacturer:        FSP-GROUP
>>    Model:        FSP520-20RAB
>>    Revision:        (null)
>>    Built on:
>>    Serial:
>>    IC Device:        PIC24FJ32GA004
>>
>> PMBus revisions (0x22):    part I, ver 1.1; part II, ver 1.2
>> Capabilities (0x90):    PEC, SMBALERT#, 100 KHz
>>
>> Supported Commands:
>>    00 page                      rw u8 (bitmask)
>>    01 operation                 rw u8 (bitmask)
>>    02 on_off_config             rw u8 (bitmask)
>>    03 clear_fault                w nodata
>>    05 page_plus_read             w block
>>    19 capability                r  u8 (bitmask)
>>    1a query                     rw process_call
> 
> pmbus_peek uses the query command to determine if a command
> is supported or not. It doesn't try to read other commands.
> 
>>    1b smbalert_mask             rw block
>>    20 vout_mode                 r  u8 (bitmask)
>>    30 coefficients              r  process_call
>>    3b fan_command_1             rw s16 (LINEAR)
>>    79 status_word               r  u16 (bitmask)
>>    7a status_vout               r  u8 (bitmask)
>>    7b status_iout               r  u8 (bitmask)
>>    7c status_input              r  u8 (bitmask)
>>    7d status_temperature        r  u8 (bitmask)
>>    81 status_fans_1_2           rw u8 (bitmask)
>>    86 read_ein                  r  block(6), Energy counter (DIRECT)
>>    87 read_eout                 r  block(6), Energy counter (DIRECT)
>>    88 read_vin                  r  s16 (LINEAR), Volts
>>    89 read_iin                  r  s16 (LINEAR), Amperes
>>    8b read_vout                 r  x16 (VOUT_MODE), Volts
>>    8c read_iout                 r  s16 (LINEAR), Amperes
>>    8d read_temperature_1        r  s16 (LINEAR), degrees Celsius
>>    8e read_temperature_2        r  s16 (LINEAR), degrees Celsius
>>    90 read_fan_speed_1          r  s16 (LINEAR)
>>    95 read_frequency            r  s16 (LINEAR)
>>    96 read_pout                 r  s16 (LINEAR), Watts
>>    97 read_pin                  r  s16 (LINEAR), Watts
>>    98 pmbus_revision            r  u8 (bitmask)
>>    99 mfr_id                    r  block, ISO 8859/1 string
>>    9a mfr_model                 r  block, ISO 8859/1 string
>>    9d mfr_date                  r  block, ISO 8859/1 string
>>    9e mfr_serial                r  block, ISO 8859/1 string
> 
> That also means it does not try to read mfr_revision or mfr_location
> since that is not supported. The PMBus driver in the kernel does
> try to read those (it doesn't use the query command to determine
> if a command is supported or not).
> 
> My suspicion is that the power supply returns something (but not
> valid SMBus block data) when reading those commands. If modifying
> the kernel to find out is not easy, another option might be to
> enable smbus tracing. Would it be possible to do that ?
> Another option might be to modify the pmbus_peek command to read
> those registers, or maybe even better to use i2cget to read the
> data directly.

Hmm, doesn't seem to trigger it at least with pmbus_peek.c, the following diff still doesn't hit the 
too big prints:

diff -U5 orig.c pmbus_peek.c
--- orig.c	2025-04-19 19:28:44.314887951 +0000
+++ pmbus_peek.c	2025-04-19 19:26:42.721194496 +0000
@@ -619,16 +619,19 @@
  	}

  	if (data.block[0] <= read_len) {
  		if (data.block[0] > 32) {
  			/* NOTE:  this probably won't be visible */
+fprintf(stderr, "\nERR too big 32!\n");
  			retval = -EFBIG;
  			goto try_i2c;
  		}
  		retval = read_len = data.block[0];
-	} else
+	} else {
+fprintf(stderr, "\nERR too big!\n");
  		retval = -E2BIG;
+}
  	memcpy(read_buf, &data.block[1], read_len);

  	return retval;

  try_i2c:
@@ -1016,11 +1019,11 @@
  	 *
  	 * NOTE:  this assumes we can check prefixes for PMB_MFR_EXT(x)
  	 * and PMB_EXT(x) operations, even though we can't query those
  	 * operations directly...
  	 */
-	if (is_pmb_extended(cmd))
+	//if (is_pmb_extended(cmd))
  		return -1;

  	if (pmdev->op[PMB_QUERY] == &unsupported || pmdev->no_query)
  		return -1;

@@ -1044,12 +1047,12 @@
  	int status;

  	/* For non-queryable devices we'll still try to read inventory
  	 * data strings.  That should be harmless but informative.
  	 */
-	if (checksupport(pmdev, cmd) == 0)
-		return NULL;
+	//if (checksupport(pmdev, cmd) == 0)
+	//	return NULL;

  	memset(buf, 0, sizeof buf);
  	status = pmbus_read_block(pmdev, cmd, sizeof buf - 1, buf);
  	return (status > 0) ? strdup((void *)buf) : NULL;
  }

> Somewhat unrelated to this, it might be time to revisit using the
> QUERY command to determine what is supported. If only I had an endless
> amount of time ... oh well. When I wrote the kernel driver, none of
> the chips I had available at the time supported that command :-(.
> 
> Thanks,
> Guenter
> 
> 

Re: PMBus memory overflow

[Guenter Roeck]

On 4/19/25 12:29, Matt Corallo wrote:

> Hmm, doesn't seem to trigger it at least with pmbus_peek.c, the following diff still doesn't hit the too big prints:
> 

Only idea I have at this point is to either enable smbus tracing in the kernel
or (better) to add test code into i2c_smbus_read_block_data() to see what
exactly triggers the problem.

Guenter

Re: PMBus memory overflow

[Guenter Roeck]

On 4/19/25 15:38, Guenter Roeck wrote:
> On 4/19/25 12:29, Matt Corallo wrote:
> 
>> Hmm, doesn't seem to trigger it at least with pmbus_peek.c, the following diff still doesn't hit the too big prints:
>>
> 
> Only idea I have at this point is to either enable smbus tracing in the kernel
> or (better) to add test code into i2c_smbus_read_block_data() to see what
> exactly triggers the problem.
> 

You could try this:

diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index e73afbefe222..b2856f88431d 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -233,6 +233,9 @@ s32 i2c_smbus_read_block_data(const struct i2c_client *client, u8 command,
         if (status)
                 return status;

+       if (data.block[0] > I2C_SMBUS_BLOCK_MAX)
+               return -E2BIG;
+
         memcpy(values, &data.block[1], data.block[0]);
         return data.block[0];

and maybe temporarily dump the entire buffer if that is seen.

diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index e73afbefe222..3dcb8b6b2427 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -233,6 +233,12 @@ s32 i2c_smbus_read_block_data(const struct i2c_client *client, u8 command,
         if (status)
                 return status;

+       if (data.block[0] > I2C_SMBUS_BLOCK_MAX) {
+               print_hex_dump_bytes("Bad SMBus block data: ", DUMP_PREFIX_NONE,
+                                    data.block, I2C_SMBUS_BLOCK_MAX);
+               return -E2BIG;
+       }
+
         memcpy(values, &data.block[1], data.block[0]);
         return data.block[0];
  }

If that doesn't trigger I am going to be really puzzled.

Guenter

Re: PMBus memory overflow

[Matt Corallo]

On 4/19/25 6:49 PM, Guenter Roeck wrote:
> On 4/19/25 15:38, Guenter Roeck wrote:
>> On 4/19/25 12:29, Matt Corallo wrote:
>>
>>> Hmm, doesn't seem to trigger it at least with pmbus_peek.c, the following diff still doesn't hit 
>>> the too big prints:
>>>
>>
>> Only idea I have at this point is to either enable smbus tracing in the kernel
>> or (better) to add test code into i2c_smbus_read_block_data() to see what
>> exactly triggers the problem.
>>
> 
> You could try this:
> 
> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
> index e73afbefe222..b2856f88431d 100644
> --- a/drivers/i2c/i2c-core-smbus.c
> +++ b/drivers/i2c/i2c-core-smbus.c
> @@ -233,6 +233,9 @@ s32 i2c_smbus_read_block_data(const struct i2c_client *client, u8 command,
>          if (status)
>                  return status;
> 
> +       if (data.block[0] > I2C_SMBUS_BLOCK_MAX)
> +               return -E2BIG;
> +
>          memcpy(values, &data.block[1], data.block[0]);
>          return data.block[0];
> 
> and maybe temporarily dump the entire buffer if that is seen.
> 
> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
> index e73afbefe222..3dcb8b6b2427 100644
> --- a/drivers/i2c/i2c-core-smbus.c
> +++ b/drivers/i2c/i2c-core-smbus.c
> @@ -233,6 +233,12 @@ s32 i2c_smbus_read_block_data(const struct i2c_client *client, u8 command,
>          if (status)
>                  return status;
> 
> +       if (data.block[0] > I2C_SMBUS_BLOCK_MAX) {
> +               print_hex_dump_bytes("Bad SMBus block data: ", DUMP_PREFIX_NONE,
> +                                    data.block, I2C_SMBUS_BLOCK_MAX);
> +               return -E2BIG;
> +       }
> +
>          memcpy(values, &data.block[1], data.block[0]);
>          return data.block[0];
>   }
> 
> If that doesn't trigger I am going to be really puzzled.

With this patch the device works fine. With debug enabled I see:

[  273.730583] i2c-core: driver [pmbus] registered
[  276.585423] pmbus 1-0058: probe
[  278.925878] Bad SMBus block data: ff ff b8 ff ff ff ff ff ff ff ff ff ff ff ff ff  ................
[  278.925881] Bad SMBus block data: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff  ................
[  278.937859] Bad SMBus block data: ff ff da ff ff ff ff ff ff ff ff ff ff ff ff ff  ................
[  278.937861] Bad SMBus block data: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff  ................
[  280.353885] i2c i2c-1: client [pmbus] registered with bus id 1-0058
[  280.353888] i2c i2c-1: new_device: Instantiated device pmbus at 0x58
matt@psu-kern:~$ sensors
pmbus-i2c-1-58
Adapter: CP2112 SMBus Bridge on hidraw1
vin:         123.00 V  (crit min =  -0.50 V, min =  -0.50 V)  ALARM (MIN)
                        (max =  -0.50 V, crit max =  -0.50 V)
vcap:        -500.00 mV
vout1:        12.05 V  (crit min = +128.00 V, min = +128.00 V)
                        (max = +128.00 V, crit max = +128.00 V)
fan1:        5304 RPM
fan2:          -1 RPM
fan3:           FAULT  ALARM
fan4:           FAULT  ALARM
temp1:        +27.0°C  (low  =  -0.5°C, high =  -0.5°C)
                        (crit low =  -0.5°C, crit =  -0.5°C)
temp2:        +32.0°C  (low  =  -0.5°C, high =  -0.5°C)
                        (crit low =  -0.5°C, crit =  -0.5°C)
temp3:         -0.5°C  (low  =  -0.5°C, high =  -0.5°C)
                        (crit low =  -0.5°C, crit =  -0.5°C)
pin:         102.00 W  (max =   1.60 kW)
pout1:        88.00 W  (max =   0.00 W, crit = -500.00 mW)
                        (cap = -500.00 mW)
iin:         988.00 mA (max =  -0.01 A, crit max =  -0.50 A)
iout1:         7.39 A  (crit min =  -0.50 A, max =  -0.50 A)
                        (crit max =  -0.50 A)

Re: PMBus memory overflow

[Guenter Roeck]

On 4/19/25 19:29, Matt Corallo wrote:
> 
> 
> On 4/19/25 6:49 PM, Guenter Roeck wrote:
>> On 4/19/25 15:38, Guenter Roeck wrote:
>>> On 4/19/25 12:29, Matt Corallo wrote:
>>>
>>>> Hmm, doesn't seem to trigger it at least with pmbus_peek.c, the following diff still doesn't hit the too big prints:
>>>>
>>>
>>> Only idea I have at this point is to either enable smbus tracing in the kernel
>>> or (better) to add test code into i2c_smbus_read_block_data() to see what
>>> exactly triggers the problem.
>>>
>>
>> You could try this:
>>
>> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
>> index e73afbefe222..b2856f88431d 100644
>> --- a/drivers/i2c/i2c-core-smbus.c
>> +++ b/drivers/i2c/i2c-core-smbus.c
>> @@ -233,6 +233,9 @@ s32 i2c_smbus_read_block_data(const struct i2c_client *client, u8 command,
>>          if (status)
>>                  return status;
>>
>> +       if (data.block[0] > I2C_SMBUS_BLOCK_MAX)
>> +               return -E2BIG;
>> +
>>          memcpy(values, &data.block[1], data.block[0]);
>>          return data.block[0];
>>
>> and maybe temporarily dump the entire buffer if that is seen.
>>
>> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
>> index e73afbefe222..3dcb8b6b2427 100644
>> --- a/drivers/i2c/i2c-core-smbus.c
>> +++ b/drivers/i2c/i2c-core-smbus.c
>> @@ -233,6 +233,12 @@ s32 i2c_smbus_read_block_data(const struct i2c_client *client, u8 command,
>>          if (status)
>>                  return status;
>>
>> +       if (data.block[0] > I2C_SMBUS_BLOCK_MAX) {
>> +               print_hex_dump_bytes("Bad SMBus block data: ", DUMP_PREFIX_NONE,
>> +                                    data.block, I2C_SMBUS_BLOCK_MAX);
>> +               return -E2BIG;
>> +       }
>> +
>>          memcpy(values, &data.block[1], data.block[0]);
>>          return data.block[0];
>>   }
>>
>> If that doesn't trigger I am going to be really puzzled.
> 
> With this patch the device works fine. With debug enabled I see:
> 
> [  273.730583] i2c-core: driver [pmbus] registered
> [  276.585423] pmbus 1-0058: probe
> [  278.925878] Bad SMBus block data: ff ff b8 ff ff ff ff ff ff ff ff ff ff ff ff ff  ................
> [  278.925881] Bad SMBus block data: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff  ................
> [  278.937859] Bad SMBus block data: ff ff da ff ff ff ff ff ff ff ff ff ff ff ff ff  ................
> [  278.937861] Bad SMBus block data: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff  ................
> [  280.353885] i2c i2c-1: client [pmbus] registered with bus id 1-0058
> [  280.353888] i2c i2c-1: new_device: Instantiated device pmbus at 0x58

Great, thanks for confirming. So the two problems are:

- The cp2112 driver does not validate the first data byte (the length field) when
   receiving SMBus block data from connected devices.
- The I2C (SMBus) core does not validate the first data byte before copying
   the data buffer.

Copying the I2C subsystem mailing list and the I2C maintainer.

Wolfram, what do you suggest ? Fixing the cp2112 driver is obviously necessary, but
I do wonder if a check such as the one above would be appropriate as well, possibly
even combined with a WARN_ONCE().

Thanks,
Guenter

> matt@psu-kern:~$ sensors
> pmbus-i2c-1-58
> Adapter: CP2112 SMBus Bridge on hidraw1
> vin:         123.00 V  (crit min =  -0.50 V, min =  -0.50 V)  ALARM (MIN)
>                         (max =  -0.50 V, crit max =  -0.50 V)
> vcap:        -500.00 mV
> vout1:        12.05 V  (crit min = +128.00 V, min = +128.00 V)
>                         (max = +128.00 V, crit max = +128.00 V)
> fan1:        5304 RPM
> fan2:          -1 RPM
> fan3:           FAULT  ALARM
> fan4:           FAULT  ALARM
> temp1:        +27.0°C  (low  =  -0.5°C, high =  -0.5°C)
>                         (crit low =  -0.5°C, crit =  -0.5°C)
> temp2:        +32.0°C  (low  =  -0.5°C, high =  -0.5°C)
>                         (crit low =  -0.5°C, crit =  -0.5°C)
> temp3:         -0.5°C  (low  =  -0.5°C, high =  -0.5°C)
>                         (crit low =  -0.5°C, crit =  -0.5°C)
> pin:         102.00 W  (max =   1.60 kW)
> pout1:        88.00 W  (max =   0.00 W, crit = -500.00 mW)
>                         (cap = -500.00 mW)
> iin:         988.00 mA (max =  -0.01 A, crit max =  -0.50 A)
> iout1:         7.39 A  (crit min =  -0.50 A, max =  -0.50 A)
>                         (crit max =  -0.50 A)

Re: PMBus memory overflow

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 413 bytes --]


> Wolfram, what do you suggest ? Fixing the cp2112 driver is obviously necessary, but
> I do wonder if a check such as the one above would be appropriate as well, possibly
> even combined with a WARN_ONCE().

How annoying, there was still an unchecked case left? Sorry. Yes, the
core can have a check for a short-term solution. The long-term solution
is to support SMBUS3.x which allows for 255 byte transfers.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: PMBus memory overflow

[Matt Corallo]

On 4/25/25 4:16 AM, Wolfram Sang wrote:
> 
>> Wolfram, what do you suggest ? Fixing the cp2112 driver is obviously necessary, but
>> I do wonder if a check such as the one above would be appropriate as well, possibly
>> even combined with a WARN_ONCE().
> 
> How annoying, there was still an unchecked case left? Sorry. Yes, the
> core can have a check for a short-term solution. The long-term solution
> is to support SMBUS3.x which allows for 255 byte transfers.

Thanks!

Any update here? I guess we already have a patch so no use in me trying to write one. Would be nice 
to get this in a pull so it can head through backports.

Thanks,
Matt

Re: PMBus memory overflow

[Guenter Roeck]

On 5/5/25 13:41, Matt Corallo wrote:
> 
> 
> On 4/25/25 4:16 AM, Wolfram Sang wrote:
>>
>>> Wolfram, what do you suggest ? Fixing the cp2112 driver is obviously necessary, but
>>> I do wonder if a check such as the one above would be appropriate as well, possibly
>>> even combined with a WARN_ONCE().
>>
>> How annoying, there was still an unchecked case left? Sorry. Yes, the
>> core can have a check for a short-term solution. The long-term solution
>> is to support SMBUS3.x which allows for 255 byte transfers.
> 
> Thanks!
> 
> Any update here? I guess we already have a patch so no use in me trying to write one. Would be nice to get this in a pull so it can head through backports.
> 

Not from my side, sorry. I am deeply buried in work and don't have time for anything
that isn't super-urgent :-(

Guenter

Re: PMBus memory overflow

[Matt Corallo]

On 5/5/25 4:50 PM, Guenter Roeck wrote:
> On 5/5/25 13:41, Matt Corallo wrote:
>>
>>
>> On 4/25/25 4:16 AM, Wolfram Sang wrote:
>>>
>>>> Wolfram, what do you suggest ? Fixing the cp2112 driver is obviously necessary, but
>>>> I do wonder if a check such as the one above would be appropriate as well, possibly
>>>> even combined with a WARN_ONCE().
>>>
>>> How annoying, there was still an unchecked case left? Sorry. Yes, the
>>> core can have a check for a short-term solution. The long-term solution
>>> is to support SMBUS3.x which allows for 255 byte transfers.
>>
>> Thanks!
>>
>> Any update here? I guess we already have a patch so no use in me trying to write one. Would be 
>> nice to get this in a pull so it can head through backports.
>>
> 
> Not from my side, sorry. I am deeply buried in work and don't have time for anything
> that isn't super-urgent :-(

Mmm, shame, its kinda annoying to leave a buffer overflow reachable from a malicious USB device 
sitting around (okay, with the default hardening configs it gets caught, but still). Can we just 
land the above patch from Wolfram to check the length before writing the buffer? Happy to clean it 
up as a formal patch submission if its easier for you.

Thanks,
Matt

Re: PMBus memory overflow

[Guenter Roeck]

On 5/5/25 13:57, Matt Corallo wrote:
> 
> 
> On 5/5/25 4:50 PM, Guenter Roeck wrote:
>> On 5/5/25 13:41, Matt Corallo wrote:
>>>
>>>
>>> On 4/25/25 4:16 AM, Wolfram Sang wrote:
>>>>
>>>>> Wolfram, what do you suggest ? Fixing the cp2112 driver is obviously necessary, but
>>>>> I do wonder if a check such as the one above would be appropriate as well, possibly
>>>>> even combined with a WARN_ONCE().
>>>>
>>>> How annoying, there was still an unchecked case left? Sorry. Yes, the
>>>> core can have a check for a short-term solution. The long-term solution
>>>> is to support SMBUS3.x which allows for 255 byte transfers.
>>>
>>> Thanks!
>>>
>>> Any update here? I guess we already have a patch so no use in me trying to write one. Would be nice to get this in a pull so it can head through backports.
>>>
>>
>> Not from my side, sorry. I am deeply buried in work and don't have time for anything
>> that isn't super-urgent :-(
> 
> Mmm, shame, its kinda annoying to leave a buffer overflow reachable from a malicious USB device sitting around (okay, with the default hardening configs it gets caught, but still). Can we just land the above patch from Wolfram to check the length before writing the buffer? Happy to clean it up as a formal patch submission if its easier for you.
> 

Please go ahead.

Thanks,
Guenter

Re: PMBus memory overflow

[Matt Corallo]

Adding security@kernel.org cause probably they should make sure this gets fixed.

On 5/5/25 9:39 PM, Guenter Roeck wrote:
> On 5/5/25 13:57, Matt Corallo wrote:
>>
>>
>> On 5/5/25 4:50 PM, Guenter Roeck wrote:
>>> On 5/5/25 13:41, Matt Corallo wrote:
>>>>
>>>>
>>>> On 4/25/25 4:16 AM, Wolfram Sang wrote:
>>>>>
>>>>>> Wolfram, what do you suggest ? Fixing the cp2112 driver is obviously necessary, but
>>>>>> I do wonder if a check such as the one above would be appropriate as well, possibly
>>>>>> even combined with a WARN_ONCE().
>>>>>
>>>>> How annoying, there was still an unchecked case left? Sorry. Yes, the
>>>>> core can have a check for a short-term solution. The long-term solution
>>>>> is to support SMBUS3.x which allows for 255 byte transfers.
>>>>
>>>> Thanks!
>>>>
>>>> Any update here? I guess we already have a patch so no use in me trying to write one. Would be 
>>>> nice to get this in a pull so it can head through backports.
>>>>
>>>
>>> Not from my side, sorry. I am deeply buried in work and don't have time for anything
>>> that isn't super-urgent :-(
>>
>> Mmm, shame, its kinda annoying to leave a buffer overflow reachable from a malicious USB device 
>> sitting around (okay, with the default hardening configs it gets caught, but still). Can we just 
>> land the above patch from Wolfram to check the length before writing the buffer? Happy to clean it 
>> up as a formal patch submission if its easier for you.
>>
> 
> Please go ahead.
> 
> Thanks,
> Guenter
> 

Re: PMBus memory overflow

[Greg KH]

On Fri, Jun 06, 2025 at 04:57:37PM -0400, Matt Corallo wrote:
> Adding security@kernel.org cause probably they should make sure this gets fixed.

That's not how security@k.o works, sorry.  As this is already public, no
need for security@k.o to get involved at all, the normal development
process happens here now.

So, submit a patch and people will be glad to review it!

thanks,

greg k-h

Re: PMBus memory overflow

[Matt Corallo]

On 6/7/25 4:19 AM, Greg KH wrote:
> On Fri, Jun 06, 2025 at 04:57:37PM -0400, Matt Corallo wrote:
>> Adding security@kernel.org cause probably they should make sure this gets fixed.
> 
> That's not how security@k.o works, sorry.  As this is already public, no
> need for security@k.o to get involved at all, the normal development
> process happens here now.
> 
> So, submit a patch and people will be glad to review it!

Thanks, figured I'd ask. Sadly there is a patch that folks seem to be okay with to fix a buffer 
overflow but its just sitting.

Matt

Re: PMBus memory overflow

[Greg KH]

On Sat, Jun 07, 2025 at 09:25:44AM -0400, Matt Corallo wrote:
> 
> 
> On 6/7/25 4:19 AM, Greg KH wrote:
> > On Fri, Jun 06, 2025 at 04:57:37PM -0400, Matt Corallo wrote:
> > > Adding security@kernel.org cause probably they should make sure this gets fixed.
> > 
> > That's not how security@k.o works, sorry.  As this is already public, no
> > need for security@k.o to get involved at all, the normal development
> > process happens here now.
> > 
> > So, submit a patch and people will be glad to review it!
> 
> Thanks, figured I'd ask. Sadly there is a patch that folks seem to be okay
> with to fix a buffer overflow but its just sitting.

Have a pointer to that patch on lore for the maintainers involved to
review?  Note, we are in the middle of the merge window, so no new
changes can be added to our trees until -rc1 is out.

thanks,

greg k-h

Re: PMBus memory overflow

[Matt Corallo]

On 6/8/25 3:14 AM, Greg KH wrote:
> On Sat, Jun 07, 2025 at 09:25:44AM -0400, Matt Corallo wrote:
>>
>>
>> On 6/7/25 4:19 AM, Greg KH wrote:
>>> On Fri, Jun 06, 2025 at 04:57:37PM -0400, Matt Corallo wrote:
>>>> Adding security@kernel.org cause probably they should make sure this gets fixed.
>>>
>>> That's not how security@k.o works, sorry.  As this is already public, no
>>> need for security@k.o to get involved at all, the normal development
>>> process happens here now.
>>>
>>> So, submit a patch and people will be glad to review it!
>>
>> Thanks, figured I'd ask. Sadly there is a patch that folks seem to be okay
>> with to fix a buffer overflow but its just sitting.
> 
> Have a pointer to that patch on lore for the maintainers involved to
> review?  Note, we are in the middle of the merge window, so no new
> changes can be added to our trees until -rc1 is out.

A proposed patch was posted by Guenter, and tested and confirmed that it fixes the issue by myself, 
at https://lore.kernel.org/linux-hwmon/284466fd-39e8-419e-8af5-41dbabb788af@roeck-us.net/ . Wolfram 
suggested this patch was acceptable at https://lore.kernel.org/linux-hwmon/aAtEydwUfVcE0XeA@shikoro/ 
but that's the last he chimed in on this issue.

Matt

Re: PMBus memory overflow

[Matt Corallo]

On 6/9/25 9:57 AM, Matt Corallo wrote:
> 
> 
> On 6/8/25 3:14 AM, Greg KH wrote:
>> On Sat, Jun 07, 2025 at 09:25:44AM -0400, Matt Corallo wrote:
>>>
>>>
>>> On 6/7/25 4:19 AM, Greg KH wrote:
>>>> On Fri, Jun 06, 2025 at 04:57:37PM -0400, Matt Corallo wrote:
>>>>> Adding security@kernel.org cause probably they should make sure this gets fixed.
>>>>
>>>> That's not how security@k.o works, sorry.  As this is already public, no
>>>> need for security@k.o to get involved at all, the normal development
>>>> process happens here now.
>>>>
>>>> So, submit a patch and people will be glad to review it!
>>>
>>> Thanks, figured I'd ask. Sadly there is a patch that folks seem to be okay
>>> with to fix a buffer overflow but its just sitting.
>>
>> Have a pointer to that patch on lore for the maintainers involved to
>> review?  Note, we are in the middle of the merge window, so no new
>> changes can be added to our trees until -rc1 is out.
> 
> A proposed patch was posted by Guenter, and tested and confirmed that it fixes the issue by myself, 
> at https://lore.kernel.org/linux-hwmon/284466fd-39e8-419e-8af5-41dbabb788af@roeck-us.net/ . Wolfram 
> suggested this patch was acceptable at https://lore.kernel.org/linux-hwmon/aAtEydwUfVcE0XeA@shikoro/ 
> but that's the last he chimed in on this issue.

Any update on getting this patch applied Wolfram? Looks like the buffer overflow is still present on 
at least 6.18.

Matt

Re: PMBus memory overflow

[Kees Cook]

On March 1, 2026 5:46:33 AM PST, Matt Corallo <yalbrymrb@mattcorallo.com> wrote:
>
>
>On 6/9/25 9:57 AM, Matt Corallo wrote:
>> 
>> 
>> On 6/8/25 3:14 AM, Greg KH wrote:
>>> On Sat, Jun 07, 2025 at 09:25:44AM -0400, Matt Corallo wrote:
>>>> 
>>>> 
>>>> On 6/7/25 4:19 AM, Greg KH wrote:
>>>>> On Fri, Jun 06, 2025 at 04:57:37PM -0400, Matt Corallo wrote:
>>>>>> Adding security@kernel.org cause probably they should make sure this gets fixed.
>>>>> 
>>>>> That's not how security@k.o works, sorry.  As this is already public, no
>>>>> need for security@k.o to get involved at all, the normal development
>>>>> process happens here now.
>>>>> 
>>>>> So, submit a patch and people will be glad to review it!
>>>> 
>>>> Thanks, figured I'd ask. Sadly there is a patch that folks seem to be okay
>>>> with to fix a buffer overflow but its just sitting.
>>> 
>>> Have a pointer to that patch on lore for the maintainers involved to
>>> review?  Note, we are in the middle of the merge window, so no new
>>> changes can be added to our trees until -rc1 is out.
>> 
>> A proposed patch was posted by Guenter, and tested and confirmed that it fixes the issue by myself, at https://lore.kernel.org/linux-hwmon/284466fd-39e8-419e-8af5-41dbabb788af@roeck-us.net/ . Wolfram suggested this patch was acceptable at https://lore.kernel.org/linux-hwmon/aAtEydwUfVcE0XeA@shikoro/ but that's the last he chimed in on this issue.
>
>Any update on getting this patch applied Wolfram? Looks like the buffer overflow is still present on at least 6.18.

Looking at the code, I think probably the best place to check would be in i2c_smbus_read_block_data() when it does a I2C_SMBUS_BLOCK_DATA cmd, since the callers are all already checking the returned status.

-- 
Kees Cook

Re: PMBus memory overflow

[Matt Corallo]

On 3/1/26 11:12 AM, Kees Cook wrote:
> 
> 
> On March 1, 2026 5:46:33 AM PST, Matt Corallo <yalbrymrb@mattcorallo.com> wrote:
>>
>>
>> On 6/9/25 9:57 AM, Matt Corallo wrote:
>>>
>>>
>>> On 6/8/25 3:14 AM, Greg KH wrote:
>>>> Have a pointer to that patch on lore for the maintainers involved to
>>>> review?  Note, we are in the middle of the merge window, so no new
>>>> changes can be added to our trees until -rc1 is out.
>>>
>>> A proposed patch was posted by Guenter, and tested and confirmed that it fixes the issue by myself, at https://lore.kernel.org/linux-hwmon/284466fd-39e8-419e-8af5-41dbabb788af@roeck-us.net/ . Wolfram suggested this patch was acceptable at https://lore.kernel.org/linux-hwmon/aAtEydwUfVcE0XeA@shikoro/ but that's the last he chimed in on this issue.
>>
>> Any update on getting this patch applied Wolfram? Looks like the buffer overflow is still present on at least 6.18.
> 
> Looking at the code, I think probably the best place to check would be in i2c_smbus_read_block_data() when it does a I2C_SMBUS_BLOCK_DATA cmd, since the callers are all already checking the returned status.

I believe that's what the above patch does?

Matt

Re: PMBus memory overflow

[Guenter Roeck]

On 3/1/26 09:10, Matt Corallo wrote:
> 
> 
> On 3/1/26 11:12 AM, Kees Cook wrote:
>>
>>
>> On March 1, 2026 5:46:33 AM PST, Matt Corallo <yalbrymrb@mattcorallo.com> wrote:
>>>
>>>
>>> On 6/9/25 9:57 AM, Matt Corallo wrote:
>>>>
>>>>
>>>> On 6/8/25 3:14 AM, Greg KH wrote:
>>>>> Have a pointer to that patch on lore for the maintainers involved to
>>>>> review?  Note, we are in the middle of the merge window, so no new
>>>>> changes can be added to our trees until -rc1 is out.
>>>>
>>>> A proposed patch was posted by Guenter, and tested and confirmed that it fixes the issue by myself, at https://lore.kernel.org/linux-hwmon/284466fd-39e8-419e-8af5-41dbabb788af@roeck-us.net/ . Wolfram suggested this patch was acceptable at https://lore.kernel.org/linux-hwmon/aAtEydwUfVcE0XeA@shikoro/ but that's the last he chimed in on this issue.
>>>
>>> Any update on getting this patch applied Wolfram? Looks like the buffer overflow is still present on at least 6.18.
>>
>> Looking at the code, I think probably the best place to check would be in i2c_smbus_read_block_data() when it does a I2C_SMBUS_BLOCK_DATA cmd, since the callers are all already checking the returned status.
> 
> I believe that's what the above patch does?
> 

That wasn't a formal patch. I thought you had volunteered to submit one,
but my memory may defeat me and I may be wrong.

Guenter

Re: PMBus memory overflow

[Kees Cook]

On Sun, Mar 01, 2026 at 12:10:08PM -0500, Matt Corallo wrote:
> 
> 
> On 3/1/26 11:12 AM, Kees Cook wrote:
> > 
> > 
> > On March 1, 2026 5:46:33 AM PST, Matt Corallo <yalbrymrb@mattcorallo.com> wrote:
> > > 
> > > 
> > > On 6/9/25 9:57 AM, Matt Corallo wrote:
> > > > 
> > > > 
> > > > On 6/8/25 3:14 AM, Greg KH wrote:
> > > > > Have a pointer to that patch on lore for the maintainers involved to
> > > > > review?  Note, we are in the middle of the merge window, so no new
> > > > > changes can be added to our trees until -rc1 is out.
> > > > 
> > > > A proposed patch was posted by Guenter, and tested and confirmed that it fixes the issue by myself, at https://lore.kernel.org/linux-hwmon/284466fd-39e8-419e-8af5-41dbabb788af@roeck-us.net/ . Wolfram suggested this patch was acceptable at https://lore.kernel.org/linux-hwmon/aAtEydwUfVcE0XeA@shikoro/ but that's the last he chimed in on this issue.
> > > 
> > > Any update on getting this patch applied Wolfram? Looks like the buffer overflow is still present on at least 6.18.
> > 
> > Looking at the code, I think probably the best place to check would be in i2c_smbus_read_block_data() when it does a I2C_SMBUS_BLOCK_DATA cmd, since the callers are all already checking the returned status.
> 
> I believe that's what the above patch does?

Sorry, I mis-pasted. I meant within i2c_smbus_xfer (rather than
i2c_smbus_read_block_data). Like:


diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index 71eb1ef56f0c..fbca606ba35a 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -547,6 +547,10 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
 			       command, protocol, data);
 	i2c_unlock_bus(adapter, I2C_LOCK_SEGMENT);
 
+	if (WARN_ON_ONCE(res == 0 && command == I2C_SMBUS_BLOCK_DATA &&
+			 data->block[0] > I2C_SMBUS_BLOCK_MAX))
+		res = -E2BIG;
+
 	return res;
 }
 EXPORT_SYMBOL(i2c_smbus_xfer);

-- 
Kees Cook

Re: PMBus memory overflow

[Guenter Roeck]

On 3/1/26 21:09, Kees Cook wrote:
> On Sun, Mar 01, 2026 at 12:10:08PM -0500, Matt Corallo wrote:
>>
>>
>> On 3/1/26 11:12 AM, Kees Cook wrote:
>>>
>>>
>>> On March 1, 2026 5:46:33 AM PST, Matt Corallo <yalbrymrb@mattcorallo.com> wrote:
>>>>
>>>>
>>>> On 6/9/25 9:57 AM, Matt Corallo wrote:
>>>>>
>>>>>
>>>>> On 6/8/25 3:14 AM, Greg KH wrote:
>>>>>> Have a pointer to that patch on lore for the maintainers involved to
>>>>>> review?  Note, we are in the middle of the merge window, so no new
>>>>>> changes can be added to our trees until -rc1 is out.
>>>>>
>>>>> A proposed patch was posted by Guenter, and tested and confirmed that it fixes the issue by myself, at https://lore.kernel.org/linux-hwmon/284466fd-39e8-419e-8af5-41dbabb788af@roeck-us.net/ . Wolfram suggested this patch was acceptable at https://lore.kernel.org/linux-hwmon/aAtEydwUfVcE0XeA@shikoro/ but that's the last he chimed in on this issue.
>>>>
>>>> Any update on getting this patch applied Wolfram? Looks like the buffer overflow is still present on at least 6.18.
>>>
>>> Looking at the code, I think probably the best place to check would be in i2c_smbus_read_block_data() when it does a I2C_SMBUS_BLOCK_DATA cmd, since the callers are all already checking the returned status.
>>
>> I believe that's what the above patch does?
> 
> Sorry, I mis-pasted. I meant within i2c_smbus_xfer (rather than
> i2c_smbus_read_block_data). Like:
> 
> 
> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
> index 71eb1ef56f0c..fbca606ba35a 100644
> --- a/drivers/i2c/i2c-core-smbus.c
> +++ b/drivers/i2c/i2c-core-smbus.c
> @@ -547,6 +547,10 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
>   			       command, protocol, data);
>   	i2c_unlock_bus(adapter, I2C_LOCK_SEGMENT);
>   
> +	if (WARN_ON_ONCE(res == 0 && command == I2C_SMBUS_BLOCK_DATA &&
> +			 data->block[0] > I2C_SMBUS_BLOCK_MAX))
> +		res = -E2BIG;
> +
>   	return res;
>   }
>   EXPORT_SYMBOL(i2c_smbus_xfer);
> 

Agreed, that makes more sense since it covers all callers, not just
i2c_smbus_read_block_data().

Guenter