* Re: [PATCH] i2c: mux: harden i2c_mux_alloc() against integer overflows
2022-09-15 11:30 [PATCH] i2c: mux: harden i2c_mux_alloc() against integer overflows Dan Carpenter
@ 2022-09-15 13:36 ` Peter Rosin
2022-09-15 13:51 ` Gustavo A. R. Silva
` (2 subsequent siblings)
3 siblings, 0 replies; 14+ messages in thread
From: Peter Rosin @ 2022-09-15 13:36 UTC (permalink / raw)
To: Dan Carpenter
Cc: Wolfram Sang, Gustavo A. R. Silva, linux-i2c, linux-kernel,
kernel-janitors
Hi Dan,
2022-09-15 at 13:30, Dan Carpenter wrote:
> A couple years back we went through the kernel an automatically
> converted size calculations to use struct_size() instead. The
> struct_size() calculation is protected against integer overflows.
>
> However it does not make sense to use the result from struct_size()
> for additional math operations as that would negate any safeness.
Indeed. Nice catch!
> Fixes: 1f3b69b6b939 ("i2c: mux: Use struct_size() in devm_kzalloc()")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Acked-by: Peter Rosin <peda@axentia.se>
Cheers,
Peter
> ---
> drivers/i2c/i2c-mux.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> index 774507b54b57..313904be5f3b 100644
> --- a/drivers/i2c/i2c-mux.c
> +++ b/drivers/i2c/i2c-mux.c
> @@ -243,9 +243,10 @@ struct i2c_mux_core *i2c_mux_alloc(struct i2c_adapter *parent,
> int (*deselect)(struct i2c_mux_core *, u32))
> {
> struct i2c_mux_core *muxc;
> + size_t mux_size;
>
> - muxc = devm_kzalloc(dev, struct_size(muxc, adapter, max_adapters)
> - + sizeof_priv, GFP_KERNEL);
> + mux_size = struct_size(muxc, adapter, max_adapters);
> + muxc = devm_kzalloc(dev, size_add(mux_size, sizeof_priv), GFP_KERNEL);
> if (!muxc)
> return NULL;
> if (sizeof_priv)
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] i2c: mux: harden i2c_mux_alloc() against integer overflows
2022-09-15 11:30 [PATCH] i2c: mux: harden i2c_mux_alloc() against integer overflows Dan Carpenter
2022-09-15 13:36 ` Peter Rosin
@ 2022-09-15 13:51 ` Gustavo A. R. Silva
2022-09-15 14:09 ` Dan Carpenter
2022-09-16 8:01 ` Kees Cook
2022-09-21 20:13 ` Wolfram Sang
3 siblings, 1 reply; 14+ messages in thread
From: Gustavo A. R. Silva @ 2022-09-15 13:51 UTC (permalink / raw)
To: Dan Carpenter
Cc: Peter Rosin, Wolfram Sang, Gustavo A. R. Silva, linux-i2c,
linux-kernel, kernel-janitors
On Thu, Sep 15, 2022 at 02:30:58PM +0300, Dan Carpenter wrote:
> A couple years back we went through the kernel an automatically
> converted size calculations to use struct_size() instead. The
> struct_size() calculation is protected against integer overflows.
>
> However it does not make sense to use the result from struct_size()
> for additional math operations as that would negate any safeness.
Right; there most be a couple more similar cases out there. I'll
look for them and fix them. Thanks!
>
> Fixes: 1f3b69b6b939 ("i2c: mux: Use struct_size() in devm_kzalloc()")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>
--
Gustavo
> ---
> drivers/i2c/i2c-mux.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> index 774507b54b57..313904be5f3b 100644
> --- a/drivers/i2c/i2c-mux.c
> +++ b/drivers/i2c/i2c-mux.c
> @@ -243,9 +243,10 @@ struct i2c_mux_core *i2c_mux_alloc(struct i2c_adapter *parent,
> int (*deselect)(struct i2c_mux_core *, u32))
> {
> struct i2c_mux_core *muxc;
> + size_t mux_size;
>
> - muxc = devm_kzalloc(dev, struct_size(muxc, adapter, max_adapters)
> - + sizeof_priv, GFP_KERNEL);
> + mux_size = struct_size(muxc, adapter, max_adapters);
> + muxc = devm_kzalloc(dev, size_add(mux_size, sizeof_priv), GFP_KERNEL);
> if (!muxc)
> return NULL;
> if (sizeof_priv)
> --
> 2.35.1
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] i2c: mux: harden i2c_mux_alloc() against integer overflows
2022-09-15 13:51 ` Gustavo A. R. Silva
@ 2022-09-15 14:09 ` Dan Carpenter
2022-09-16 8:07 ` Kees Cook
0 siblings, 1 reply; 14+ messages in thread
From: Dan Carpenter @ 2022-09-15 14:09 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Peter Rosin, Wolfram Sang, Gustavo A. R. Silva, linux-i2c,
linux-kernel, kernel-janitors
On Thu, Sep 15, 2022 at 02:51:21PM +0100, Gustavo A. R. Silva wrote:
> On Thu, Sep 15, 2022 at 02:30:58PM +0300, Dan Carpenter wrote:
> > A couple years back we went through the kernel an automatically
> > converted size calculations to use struct_size() instead. The
> > struct_size() calculation is protected against integer overflows.
> >
> > However it does not make sense to use the result from struct_size()
> > for additional math operations as that would negate any safeness.
>
> Right; there most be a couple more similar cases out there. I'll
> look for them and fix them. Thanks!
>
That thought occured to me too. :P The main problem with that theory
is that sometimes people use struct_size() for readability instead of
just for checking for integer overflows. Also there are some places
which check for integer overflows manually before doing the math. So
this code is not perfect.
It would probaby be useful to mark passed data as explicitly unsafe for
integer overflows. Smatch already tracks user data. And if the user
data has been capped to an unknown value. But this would be a
completely separate flag which says that "this value came from
size_add/mul()".
regards,
dan carpenter
drivers/char/tpm/eventlog/tpm2.c:57 tpm2_bios_measurements_start() warn: using integer overflow function 'size_add()' for math
drivers/i2c/i2c-mux.c:248 i2c_mux_alloc() warn: using integer overflow function 'size_add()' for math
drivers/infiniband/hw/qib/qib_user_sdma.c:949 qib_user_sdma_queue_pkts() warn: using integer overflow function 'size_add()' for math
drivers/spi/spi.c:3320 spi_replace_transfers() warn: using integer overflow function 'size_add()' for math
drivers/gpu/drm/msm/msm_gem_submit.c:35 submit_create() warn: using integer overflow function 'size_add()' for math
drivers/cxl/pmem.c:151 cxl_pmem_set_config_data() warn: using integer overflow function 'size_add()' for math
drivers/md/dm-stats.c:295 dm_stats_create() warn: using integer overflow function 'size_add()' for math
drivers/md/dm-ioctl.c:1607 retrieve_deps() warn: using integer overflow function 'size_add()' for math
drivers/remoteproc/remoteproc_core.c:527 rproc_handle_vdev() warn: using integer overflow function 'size_add()' for math
drivers/rpmsg/qcom_glink_native.c:984 qcom_glink_handle_intent() warn: using integer overflow function 'size_add()' for math
drivers/net/ethernet/qlogic/qed/qed_ll2.c:1610 qed_ll2_establish_connection() warn: using integer overflow function 'size_add()' for math
drivers/net/ethernet/chelsio/cxgb4/sge.c:2551 cxgb4_ethofld_send_flowc() warn: using integer overflow function 'size_add()' for math
drivers/net/ethernet/intel/ice/ice_flex_pipe.c:2070 ice_pkg_buf_reserve_section() warn: using integer overflow function 'size_mul()' for math
drivers/net/ethernet/intel/ice/ice_switch.c:2562 ice_add_marker_act() warn: using integer overflow function 'size_add()' for math
drivers/net/ethernet/intel/ice/ice_switch.c:2567 ice_add_marker_act() warn: using integer overflow function 'size_add()' for math
drivers/net/ethernet/intel/ice/ice_switch.c:5478 ice_dummy_packet_add_vlan() warn: using integer overflow function 'size_mul()' for math
drivers/net/ethernet/intel/ice/ice_switch.c:5501 ice_dummy_packet_add_vlan() warn: using integer overflow function 'size_mul()' for math
drivers/gpio/gpiolib.c:4261 gpiod_get_array() warn: using integer overflow function 'size_add()' for math
drivers/gpio/gpiolib.c:4261 gpiod_get_array() warn: using integer overflow function 'size_add()' for math
fs/ntfs3/xattr.c:26 unpacked_ea_size() warn: using integer overflow function 'size_add()' for math
fs/ntfs3/xattr.c:291 ntfs_set_ea() warn: using integer overflow function 'size_add()' for math
io_uring/io_uring.c:2477 rings_size() warn: using integer overflow function 'size_add()' for math
kernel/module/sysfs.c:83 add_sect_attrs() warn: using integer overflow function 'size_add()' for math
kernel/irq/generic-chip.c:310 __irq_alloc_domain_generic_chips() warn: using integer overflow function 'size_add()' for math
kernel/irq/generic-chip.c:310 __irq_alloc_domain_generic_chips() warn: using integer overflow function 'size_add()' for math
kernel/dma/swiotlb.c:355 swiotlb_init_remap() warn: using integer overflow function 'size_mul()' for math
kernel/dma/swiotlb.c:476 swiotlb_exit() warn: using integer overflow function 'size_mul()' for math
sound/soc/qcom/qdsp6/q6apm.c:103 audioreach_graph_mgmt_cmd() warn: using integer overflow function 'size_add()' for math
sound/soc/qcom/qdsp6/audioreach.c:458 audioreach_populate_graph() warn: using integer overflow function 'size_add()' for math
sound/soc/qcom/qdsp6/audioreach.c:501 audioreach_alloc_graph_pkt() warn: using integer overflow function 'size_add()' for math
sound/soc/qcom/qdsp6/audioreach.c:502 audioreach_alloc_graph_pkt() warn: using integer overflow function 'size_add()' for math
sound/soc/qcom/qdsp6/audioreach.c:503 audioreach_alloc_graph_pkt() warn: using integer overflow function 'size_add()' for math
sound/soc/qcom/qdsp6/audioreach.c:505 audioreach_alloc_graph_pkt() warn: using integer overflow function 'size_add()' for math
sound/soc/qcom/qdsp6/audioreach.c:506 audioreach_alloc_graph_pkt() warn: using integer overflow function 'size_add()' for math
sound/soc/qcom/qdsp6/audioreach.c:842 audioreach_pcm_set_media_format() warn: using integer overflow function 'size_add()' for math
net/wireless/scan.c:765 cfg80211_scan_6ghz() warn: using integer overflow function 'size_add()' for math
net/tls/tls_sw.c:1486 tls_decrypt_sg() warn: using integer overflow function 'size_add()' for math
net/bridge/br_multicast.c:2770 br_ip6_multicast_mld2_report() warn: using integer overflow function 'size_add()' for math
net/bluetooth/hci_codec.c:153 hci_read_supported_codecs() warn: using integer overflow function 'size_mul()' for math
net/bluetooth/hci_codec.c:165 hci_read_supported_codecs() warn: using integer overflow function 'size_mul()' for math
net/bluetooth/hci_codec.c:172 hci_read_supported_codecs() warn: using integer overflow function 'size_mul()' for math
net/bluetooth/hci_codec.c:220 hci_read_supported_codecs_v2() warn: using integer overflow function 'size_mul()' for math
net/bluetooth/hci_codec.c:232 hci_read_supported_codecs_v2() warn: using integer overflow function 'size_mul()' for math
net/bluetooth/hci_codec.c:239 hci_read_supported_codecs_v2() warn: using integer overflow function 'size_mul()' for math
lib/stackdepot.c:125 depot_alloc_stack() warn: using integer overflow function 'size_add()' for math
mm/percpu.c:2444 pcpu_alloc_alloc_info() warn: using integer overflow function 'size_add()' for math
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] i2c: mux: harden i2c_mux_alloc() against integer overflows
2022-09-15 14:09 ` Dan Carpenter
@ 2022-09-16 8:07 ` Kees Cook
2022-09-16 8:23 ` Dan Carpenter
0 siblings, 1 reply; 14+ messages in thread
From: Kees Cook @ 2022-09-16 8:07 UTC (permalink / raw)
To: Dan Carpenter
Cc: Gustavo A. R. Silva, Peter Rosin, Wolfram Sang,
Gustavo A. R. Silva, linux-i2c, linux-kernel, kernel-janitors,
linux-hardening
On Thu, Sep 15, 2022 at 05:09:45PM +0300, Dan Carpenter wrote:
> It would probaby be useful to mark passed data as explicitly unsafe for
> integer overflows. Smatch already tracks user data. And if the user
> data has been capped to an unknown value. But this would be a
> completely separate flag which says that "this value came from
> size_add/mul()".
I really want a __must_check_type(size_t) attribute or something for
functions, so we can get a subset of -Wconversion warnings, etc.
> drivers/char/tpm/eventlog/tpm2.c:57 tpm2_bios_measurements_start() warn: using integer overflow function 'size_add()' for math
> [...]
> drivers/net/ethernet/intel/ice/ice_flex_pipe.c:2070 ice_pkg_buf_reserve_section() warn: using integer overflow function 'size_mul()' for math
I see size_add() and size_mul() here. I would have expected some
size_sub() opportunities too? Did nothing pop out?
--
Kees Cook
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] i2c: mux: harden i2c_mux_alloc() against integer overflows
2022-09-16 8:07 ` Kees Cook
@ 2022-09-16 8:23 ` Dan Carpenter
2022-09-16 13:31 ` Kees Cook
0 siblings, 1 reply; 14+ messages in thread
From: Dan Carpenter @ 2022-09-16 8:23 UTC (permalink / raw)
To: Kees Cook
Cc: Gustavo A. R. Silva, Peter Rosin, Wolfram Sang,
Gustavo A. R. Silva, linux-i2c, linux-kernel, kernel-janitors,
linux-hardening
[-- Attachment #1: Type: text/plain, Size: 1127 bytes --]
On Fri, Sep 16, 2022 at 01:07:25AM -0700, Kees Cook wrote:
> On Thu, Sep 15, 2022 at 05:09:45PM +0300, Dan Carpenter wrote:
> > It would probaby be useful to mark passed data as explicitly unsafe for
> > integer overflows. Smatch already tracks user data. And if the user
> > data has been capped to an unknown value. But this would be a
> > completely separate flag which says that "this value came from
> > size_add/mul()".
>
> I really want a __must_check_type(size_t) attribute or something for
> functions, so we can get a subset of -Wconversion warnings, etc.
>
I have a list of these. Attached.
> > drivers/char/tpm/eventlog/tpm2.c:57 tpm2_bios_measurements_start() warn: using integer overflow function 'size_add()' for math
> > [...]
> > drivers/net/ethernet/intel/ice/ice_flex_pipe.c:2070 ice_pkg_buf_reserve_section() warn: using integer overflow function 'size_mul()' for math
>
> I see size_add() and size_mul() here. I would have expected some
> size_sub() opportunities too? Did nothing pop out?
I didn't look at size_sub(). I'll add it to the mix and report back on
Monday.
regards,
dan carpenter
[-- Attachment #2: err-list --]
[-- Type: text/plain, Size: 10543 bytes --]
drivers/i2c/muxes/i2c-mux-pinctrl.c:96 i2c_mux_pinctrl_probe() saving 'size_add' to type 'int'
drivers/i2c/muxes/i2c-mux-gpio.c:156 i2c_mux_gpio_probe() saving 'size_mul' to type 'int'
drivers/firmware/efi/efi.c:655 efi_config_parse_tables() saving 'size_add' to type 'ullong'
drivers/staging/rtl8723bs/os_dep/osdep_service.c:227 rtw_cbuf_alloc() saving 'size_add' to type 'uint'
drivers/i3c/master.c:928 i3c_master_defslvs_locked() saving 'size_add' to type 'ushort'
drivers/isdn/hardware/mISDN/hfcsusb.c:264 hfcsusb_ph_info() saving 'size_add' to type 'uint'
drivers/gpu/drm/i915/i915_query.c:146 query_engine_info() saving 'size_add' to type 'int'
drivers/gpu/drm/nouveau/nouveau_svm.c:930 nouveau_pfns_map() saving 'size_add' to type 'uint'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c:525 amdgpu_vm_pt_create() saving 'size_add' to type 'uint'
drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c:527 amdgpu_discovery_read_harvest_bit_per_ip() saving 'size_add' to type 'ushort'
drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c:1186 amdgpu_discovery_reg_base_init() saving 'size_add' to type 'ushort'
drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c:1236 amdgpu_discovery_get_ip_version() saving 'size_add' to type 'ushort'
drivers/nvme/target/admin-cmd.c:267 nvmet_format_ana_group() saving 'size_add' to type 'uint'
drivers/nvme/host/fc.c:2924 nvme_fc_create_io_queues() saving 'size_add' to type 'uint'
drivers/nvme/host/fc.c:3555 nvme_fc_init_ctrl() saving 'size_add' to type 'uint'
drivers/cxl/acpi.c:58 cxl_acpi_cfmws_verify() saving 'size_add' to type 'int'
drivers/acpi/prmt.c:106 acpi_parse_prmt() saving 'size_add' to type 'uint'
drivers/acpi/prmt.c:126 acpi_parse_prmt() saving 'size_add' to type 'ullong'
drivers/dma/ioat/dca.c:279 ioat_dca_init() saving 'size_add' to type 'int'
drivers/media/test-drivers/vivid/vivid-core.c:1780 vivid_create_instance() saving 'size_mul' to type 'uint'
drivers/scsi/aacraid/aachba.c:1251 aac_read_raw_io() saving 'size_add' to type 'ushort'
drivers/scsi/aacraid/aachba.c:1382 aac_write_raw_io() saving 'size_add' to type 'ushort'
drivers/scsi/megaraid/megaraid_sas_base.c:5157 megasas_update_ext_vd_details() saving 'size_add' to type 'uint'
drivers/scsi/megaraid/megaraid_sas_fp.c:329 MR_ValidateMapInfo() saving 'size_add' to type 'uint'
drivers/scsi/virtio_scsi.c:863 virtscsi_probe() saving 'size_add' to type 'int'
drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c:720 kvaser_usb_init_one() saving 'size_add' to type 'int'
drivers/net/usb/cdc-phonet.c:354 usbpn_probe() saving 'size_add' to type 'int'
drivers/net/dsa/ocelot/felix_vsc9959.c:2233 vsc9959_psfp_filter_add() saving 'size_add' to type 'int'
drivers/net/wireless/rndis_wlan.c:1691 get_device_pmkids() saving 'size_add' to type 'int'
drivers/net/wireless/rndis_wlan.c:1724 set_device_pmkids() saving 'size_add' to type 'int'
drivers/net/wireless/rndis_wlan.c:1770 remove_pmkid() saving 'size_add' to type 'uint'
drivers/net/wireless/rndis_wlan.c:1813 update_pmkid() saving 'size_add' to type 'int'
drivers/net/wireless/zydas/zd1211rw/zd_usb.c:1890 zd_usb_iowrite16v_async() saving 'size_add' to type 'int'
drivers/net/wireless/ath/ath10k/coredump.c:1568 ath10k_coredump_build() saving 'size_add' to type 'uint'
drivers/net/wireless/ath/ath10k/wmi.c:6616 ath10k_wmi_op_gen_init() saving 'size_add' to type 'uint'
drivers/net/wireless/ath/ath10k/wmi.c:6679 ath10k_wmi_10_1_op_gen_init() saving 'size_add' to type 'uint'
drivers/net/wireless/ath/ath10k/wmi.c:6750 ath10k_wmi_10_2_op_gen_init() saving 'size_add' to type 'uint'
drivers/net/wireless/ath/ath10k/wmi.c:6844 ath10k_wmi_10_4_op_gen_init() saving 'size_add' to type 'uint'
drivers/net/wireless/ath/ath10k/wmi.c:7555 ath10k_wmi_op_gen_scan_chan_list() saving 'size_add' to type 'uint'
drivers/net/wireless/ath/ath6kl/wmi.c:1967 ath6kl_wmi_startscan_cmd() saving 'size_add' to type 'uint'
drivers/net/wireless/ath/ath6kl/wmi.c:2023 ath6kl_wmi_beginscan_cmd() saving 'size_add' to type 'uint'
drivers/net/wireless/silabs/wfx/hif_tx_mib.c:103 wfx_hif_set_beacon_filter_table() saving 'size_add' to type 'int'
drivers/net/wireless/quantenna/qtnfmac/commands.c:206 qtnf_cmd_start_ap_can_fit() saving 'size_add' to type 'uint'
drivers/net/wireless/intel/iwlwifi/mvm/d3.c:636 iwl_mvm_send_patterns_v1() saving 'size_add' to type 'ushort'
drivers/net/wireless/intel/iwlwifi/dvm/lib.c:1007 iwlagn_send_patterns() saving 'size_add' to type 'ushort'
drivers/net/wireless/intel/iwlwifi/fw/init.c:126 iwl_configure_rxq() saving 'size_add' to type 'int'
drivers/net/ethernet/freescale/enetc/enetc_qos.c:88 enetc_setup_taprio() saving 'size_add' to type 'ushort'
drivers/net/ethernet/freescale/enetc/enetc_qos.c:738 enetc_streamgate_hw_set() saving 'size_add' to type 'ushort'
drivers/net/ethernet/freescale/enetc/enetc_qos.c:1186 enetc_psfp_parse_clsflower() saving 'size_add' to type 'int'
drivers/net/ethernet/google/gve/gve_main.c:141 gve_alloc_stats_report() saving 'size_add' to type 'ullong'
drivers/net/ethernet/netronome/nfp/flower/cmsg.c:49 nfp_flower_cmsg_mac_repr_start() saving 'size_add' to type 'uint'
drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp.c:1080 nfp_nsp_read_module_eeprom() saving 'size_add' to type 'int'
drivers/net/ethernet/chelsio/cxgb4/sge.c:2550 cxgb4_ethofld_send_flowc() saving 'size_add' to type 'uint'
drivers/net/ethernet/intel/ice/ice_common.c:2022 ice_alloc_hw_res() saving 'size_add' to type 'ushort'
drivers/net/ethernet/intel/ice/ice_common.c:2059 ice_free_hw_res() saving 'size_add' to type 'ushort'
drivers/net/ethernet/intel/ice/ice_common.c:4080 ice_aq_add_lan_txq() saving 'size_add' to type 'ushort'
drivers/net/ethernet/intel/ice/ice_common.c:4164 ice_aq_dis_lan_txq() saving 'size_add' to type 'ushort'
drivers/net/ethernet/intel/ice/ice_common.c:4222 ice_aq_add_rdma_qsets() saving 'size_add' to type 'ushort'
drivers/net/ethernet/intel/ice/ice_common.c:4780 ice_ena_vsi_rdma_qset() saving 'size_add' to type 'ushort'
drivers/net/ethernet/intel/ice/ice_switch.c:2561 ice_add_marker_act() saving 'size_add' to type 'ushort'
drivers/net/ethernet/intel/ice/ice_switch.c:2701 ice_update_vsi_list_rule() saving 'size_add' to type 'ushort'
drivers/net/ethernet/intel/ice/ice_switch.c:6063 ice_add_adv_rule() saving 'size_add' to type 'ushort'
drivers/net/ethernet/intel/ice/ice_sched.c:240 ice_sched_remove_elems() saving 'size_add' to type 'ushort'
drivers/net/fddi/skfp/smt.c:1066 smt_send_sif_operation() saving 'size_add' to type 'int'
fs/btrfs/subpage.c:166 btrfs_alloc_subpage() saving 'size_add' to type 'uint'
fs/ntfs3/fslog.c:392 lrh_length() saving 'size_add' to type 'uint'
fs/ntfs3/fsntfs.c:1686 sid_length() saving 'size_add' to type 'uint'
./fs/xfs/libxfs/xfs_attr_sf.h:41 xfs_attr_sf_entsize() saving 'size_add' to type 'int'
fs/xfs/libxfs/xfs_attr_sf.h:41 xfs_attr_sf_entsize() saving 'size_add' to type 'int'
fs/erofs/zdata.c:126 z_erofs_create_pcluster_pool() saving 'size_add' to type 'uint'
fs/ocfs2/dlm/dlmrecovery.c:1124 dlm_send_mig_lockres_msg() saving 'size_add' to type 'uint'
kernel/trace/trace_events_user.c:1275 user_events_ref_add() saving 'size_add' to type 'int'
kernel/audit.c:1482 audit_receive_msg() saving 'size_add' to type 'int'
kernel/dma/swiotlb.c:361 swiotlb_init_remap() saving 'size_mul' to type 'ullong'
kernel/dma/swiotlb.c:487 swiotlb_exit() saving 'size_mul' to type 'ullong'
kernel/auditfilter.c:1095 audit_list_rules() saving 'size_add' to type 'int'
kernel/bpf/reuseport_array.c:158 reuseport_array_alloc() saving 'size_add' to type 'ullong'
sound/soc/sof/ipc4-topology.c:1408 sof_ipc4_control_load_volume() saving 'size_add' to type 'uint'
sound/soc/sof/ipc3-topology.c:1657 sof_ipc3_control_load_volume() saving 'size_add' to type 'uint'
sound/soc/sof/ipc3-topology.c:1688 sof_ipc3_control_load_enum() saving 'size_add' to type 'uint'
sound/soc/intel/avs/apl.c:25 apl_enable_logs() saving 'size_add' to type 'uint'
sound/soc/intel/avs/skl.c:24 skl_enable_logs() saving 'size_add' to type 'uint'
sound/soc/intel/skylake/skl-topology.c:869 skl_tplg_find_moduleid_from_uuid() saving 'size_add' to type 'int'
crypto/algif_aead.c:254 _aead_recvmsg() saving 'size_mul' to type 'int'
crypto/algif_skcipher.c:95 _skcipher_recvmsg() saving 'size_mul' to type 'int'
net/netfilter/ipvs/ip_vs_ctl.c:2857 do_ip_vs_get_ctl() saving 'size_add' to type 'int'
net/netfilter/ipvs/ip_vs_ctl.c:2898 do_ip_vs_get_ctl() saving 'size_add' to type 'int'
net/mac80211/cfg.c:1123 ieee80211_assign_beacon() saving 'size_add' to type 'int'
net/mac80211/cfg.c:1127 ieee80211_assign_beacon() saving 'size_add' to type 'int'
net/mac80211/cfg.c:1150 ieee80211_assign_beacon() saving 'size_add' to type 'uchar*'
net/tipc/link.c:1536 tipc_build_gap_ack_blks() saving 'size_add' to type 'ushort'
net/bridge/br_multicast.c:2768 br_ip6_multicast_mld2_report() saving 'size_add' to type 'uint'
net/ipv6/mcast.c:450 ip6_mc_source() saving 'size_add' to type 'int'
net/ipv6/mcast.c:461 ip6_mc_source() saving 'size_add' to type 'int'
net/ipv6/mcast.c:530 ip6_mc_msfilter() saving 'size_add' to type 'int'
net/ipv6/mcast.c:549 ip6_mc_msfilter() saving 'size_add' to type 'int'
net/ipv6/mcast.c:566 ip6_mc_msfilter() saving 'size_add' to type 'int'
net/ipv6/mcast.c:2607 ip6_mc_leave_src() saving 'size_add' to type 'int'
net/xdp/xskmap.c:76 xsk_map_alloc() saving 'size_add' to type 'ullong'
net/mpls/mpls_iptunnel.c:191 mpls_build_state() saving 'size_add' to type 'int'
net/sched/cls_u32.c:1295 u32_dump() saving 'size_add' to type 'int'
net/sched/cls_u32.c:1359 u32_dump() saving 'size_add' to type 'int'
net/sched/act_pedit.c:450 tcf_pedit_dump() saving 'size_add' to type 'int'
net/bluetooth/a2mp.c:170 a2mp_discover_req() saving 'size_add' to type 'ushort'
net/bluetooth/mgmt.c:2856 load_link_keys() saving 'size_add' to type 'ushort'
net/bluetooth/mgmt.c:4197 set_blocked_keys() saving 'size_add' to type 'ushort'
net/bluetooth/mgmt.c:7103 load_irks() saving 'size_add' to type 'ushort'
net/bluetooth/mgmt.c:7193 load_long_term_keys() saving 'size_add' to type 'ushort'
net/bluetooth/mgmt.c:7888 load_conn_param() saving 'size_add' to type 'ushort'
net/ipv4/igmp.c:2250 ip_mc_leave_src() saving 'size_add' to type 'int'
net/ipv4/igmp.c:2399 ip_mc_source() saving 'size_add' to type 'int'
net/ipv4/igmp.c:2411 ip_mc_source() saving 'size_add' to type 'int'
net/ipv4/igmp.c:2488 ip_mc_msfilter() saving 'size_add' to type 'int'
net/ipv4/igmp.c:2502 ip_mc_msfilter() saving 'size_add' to type 'int'
net/ipv4/igmp.c:2516 ip_mc_msfilter() saving 'size_add' to type 'int'
net/ipv4/igmp.c:2575 ip_mc_msfget() saving 'size_mul' to type 'int'
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] i2c: mux: harden i2c_mux_alloc() against integer overflows
2022-09-16 8:23 ` Dan Carpenter
@ 2022-09-16 13:31 ` Kees Cook
2022-09-16 14:55 ` Dan Carpenter
0 siblings, 1 reply; 14+ messages in thread
From: Kees Cook @ 2022-09-16 13:31 UTC (permalink / raw)
To: Dan Carpenter
Cc: Gustavo A. R. Silva, Peter Rosin, Wolfram Sang,
Gustavo A. R. Silva, linux-i2c, linux-kernel, kernel-janitors,
linux-hardening
On Fri, Sep 16, 2022 at 11:23:25AM +0300, Dan Carpenter wrote:
> [...]
> net/ipv6/mcast.c:450 ip6_mc_source() saving 'size_add' to type 'int'
Interesting! Are you able to report the consumer? e.g. I think a bunch
of these would be fixed by:
diff --git a/net/core/sock.c b/net/core/sock.c
index 4cb957d934a2..f004c4d411e3 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2552,10 +2552,11 @@ struct sk_buff *sock_omalloc(struct sock *sk, unsigned long size,
/*
* Allocate a memory block from the socket's option memory buffer.
*/
-void *sock_kmalloc(struct sock *sk, int size, gfp_t priority)
+void *sock_kmalloc(struct sock *sk, size_t size, gfp_t priority)
{
- if ((unsigned int)size <= sysctl_optmem_max &&
- atomic_read(&sk->sk_omem_alloc) + size < sysctl_optmem_max) {
+ if (size > INT_MAX || size > sysctl_optmem_max)
+ return NULL;
+ if (atomic_read(&sk->sk_omem_alloc) < sysctl_optmem_max - size) {
void *mem;
/* First do the add, to avoid the race if kmalloc
* might sleep.
--
Kees Cook
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH] i2c: mux: harden i2c_mux_alloc() against integer overflows
2022-09-16 13:31 ` Kees Cook
@ 2022-09-16 14:55 ` Dan Carpenter
2022-09-16 21:31 ` Kees Cook
0 siblings, 1 reply; 14+ messages in thread
From: Dan Carpenter @ 2022-09-16 14:55 UTC (permalink / raw)
To: Kees Cook
Cc: Gustavo A. R. Silva, Peter Rosin, Wolfram Sang,
Gustavo A. R. Silva, linux-i2c, linux-kernel, kernel-janitors,
linux-hardening
[-- Attachment #1: Type: text/plain, Size: 746 bytes --]
On Fri, Sep 16, 2022 at 06:31:45AM -0700, Kees Cook wrote:
> On Fri, Sep 16, 2022 at 11:23:25AM +0300, Dan Carpenter wrote:
> > [...]
> > net/ipv6/mcast.c:450 ip6_mc_source() saving 'size_add' to type 'int'
>
> Interesting! Are you able to report the consumer? e.g. I think a bunch
> of these would be fixed by:
>
Are you asking if I can add "passed to sock_kmalloc()" to the report?
It's possible but it's kind of a headache the way this code is written.
When you pass a function to another function in Smatch:
frob(size_add());
Then Smatch creates a fake assignment: "frob(fake_assign = size_add());"
and parses that instead. So this check only looks at the
"fake_assign = size_add();" assignment.
Attached.
regards,
dan carpenter
[-- Attachment #2: check_overflow_truncated.c --]
[-- Type: text/x-csrc, Size: 1545 bytes --]
/*
* Copyright (C) 2022 Oracle.
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
* as published by the Free Software Foundation; either version 2
* of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, see http://www.gnu.org/copyleft/gpl.txt
*/
#include "smatch.h"
#include "smatch_extra.h"
#include "smatch_slist.h"
static int my_id;
static void check_size_t(const char *fn, struct expression *expr, void *unused)
{
struct symbol *type;
type = get_type(expr->left);
if (types_equiv(type, &long_ctype) ||
types_equiv(type, &ulong_ctype))
return;
sm_msg("saving '%s' to type '%s'", fn, type_to_str(type));
}
void check_overflow_truncated(int id)
{
my_id = id;
if (option_project != PROJ_KERNEL)
return;
add_function_assign_hook("size_mul", &check_size_t, NULL);
add_function_assign_hook("size_add", &check_size_t, NULL);
add_function_assign_hook("size_sub", &check_size_t, NULL);
add_function_assign_hook("__ab_c_size", &check_size_t, NULL);
add_function_assign_hook("array_size", &check_size_t, NULL);
add_function_assign_hook("array3_size", &check_size_t, NULL);
}
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] i2c: mux: harden i2c_mux_alloc() against integer overflows
2022-09-16 14:55 ` Dan Carpenter
@ 2022-09-16 21:31 ` Kees Cook
0 siblings, 0 replies; 14+ messages in thread
From: Kees Cook @ 2022-09-16 21:31 UTC (permalink / raw)
To: Dan Carpenter
Cc: Gustavo A. R. Silva, Peter Rosin, Wolfram Sang,
Gustavo A. R. Silva, linux-i2c, linux-kernel, kernel-janitors,
linux-hardening
On Fri, Sep 16, 2022 at 05:55:55PM +0300, Dan Carpenter wrote:
> On Fri, Sep 16, 2022 at 06:31:45AM -0700, Kees Cook wrote:
> > On Fri, Sep 16, 2022 at 11:23:25AM +0300, Dan Carpenter wrote:
> > > [...]
> > > net/ipv6/mcast.c:450 ip6_mc_source() saving 'size_add' to type 'int'
> >
> > Interesting! Are you able to report the consumer? e.g. I think a bunch
> > of these would be fixed by:
> >
>
> Are you asking if I can add "passed to sock_kmalloc()" to the report?
Yeah.
> It's possible but it's kind of a headache the way this code is written.
Okay, no worries -- I was curious if it would be "easy". I can happily
just spit out the source line.
--
Kees Cook
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] i2c: mux: harden i2c_mux_alloc() against integer overflows
2022-09-15 11:30 [PATCH] i2c: mux: harden i2c_mux_alloc() against integer overflows Dan Carpenter
2022-09-15 13:36 ` Peter Rosin
2022-09-15 13:51 ` Gustavo A. R. Silva
@ 2022-09-16 8:01 ` Kees Cook
2022-09-16 8:20 ` Dan Carpenter
2022-09-21 20:13 ` Wolfram Sang
3 siblings, 1 reply; 14+ messages in thread
From: Kees Cook @ 2022-09-16 8:01 UTC (permalink / raw)
To: Dan Carpenter
Cc: Peter Rosin, Wolfram Sang, Gustavo A. R. Silva, linux-i2c,
linux-kernel, kernel-janitors
On Thu, Sep 15, 2022 at 02:30:58PM +0300, Dan Carpenter wrote:
> A couple years back we went through the kernel an automatically
> converted size calculations to use struct_size() instead. The
> struct_size() calculation is protected against integer overflows.
>
> However it does not make sense to use the result from struct_size()
> for additional math operations as that would negate any safeness.
>
> Fixes: 1f3b69b6b939 ("i2c: mux: Use struct_size() in devm_kzalloc()")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> drivers/i2c/i2c-mux.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> index 774507b54b57..313904be5f3b 100644
> --- a/drivers/i2c/i2c-mux.c
> +++ b/drivers/i2c/i2c-mux.c
> @@ -243,9 +243,10 @@ struct i2c_mux_core *i2c_mux_alloc(struct i2c_adapter *parent,
> int (*deselect)(struct i2c_mux_core *, u32))
> {
> struct i2c_mux_core *muxc;
> + size_t mux_size;
>
> - muxc = devm_kzalloc(dev, struct_size(muxc, adapter, max_adapters)
> - + sizeof_priv, GFP_KERNEL);
> + mux_size = struct_size(muxc, adapter, max_adapters);
> + muxc = devm_kzalloc(dev, size_add(mux_size, sizeof_priv), GFP_KERNEL);
> if (!muxc)
> return NULL;
> if (sizeof_priv)
The new variable makes it more readable, but beyond that, do you see any
reason not to just directly compose the calls?
diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index 774507b54b57..6c481cde6517 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -244,8 +244,10 @@ struct i2c_mux_core *i2c_mux_alloc(struct i2c_adapter *parent,
{
struct i2c_mux_core *muxc;
- muxc = devm_kzalloc(dev, struct_size(muxc, adapter, max_adapters)
- + sizeof_priv, GFP_KERNEL);
+ muxc = devm_kzalloc(dev,
+ size_add(struct_size(muxc, adapter, max_adapters),
+ sizeof_priv),
+ GFP_KERNEL);
if (!muxc)
return NULL;
if (sizeof_priv)
> --
> 2.35.1
>
--
Kees Cook
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH] i2c: mux: harden i2c_mux_alloc() against integer overflows
2022-09-16 8:01 ` Kees Cook
@ 2022-09-16 8:20 ` Dan Carpenter
2022-09-16 19:31 ` Wolfram Sang
0 siblings, 1 reply; 14+ messages in thread
From: Dan Carpenter @ 2022-09-16 8:20 UTC (permalink / raw)
To: Kees Cook
Cc: Peter Rosin, Wolfram Sang, Gustavo A. R. Silva, linux-i2c,
linux-kernel, kernel-janitors
On Fri, Sep 16, 2022 at 01:01:42AM -0700, Kees Cook wrote:
> > diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> > index 774507b54b57..313904be5f3b 100644
> > --- a/drivers/i2c/i2c-mux.c
> > +++ b/drivers/i2c/i2c-mux.c
> > @@ -243,9 +243,10 @@ struct i2c_mux_core *i2c_mux_alloc(struct i2c_adapter *parent,
> > int (*deselect)(struct i2c_mux_core *, u32))
> > {
> > struct i2c_mux_core *muxc;
> > + size_t mux_size;
> >
> > - muxc = devm_kzalloc(dev, struct_size(muxc, adapter, max_adapters)
> > - + sizeof_priv, GFP_KERNEL);
> > + mux_size = struct_size(muxc, adapter, max_adapters);
> > + muxc = devm_kzalloc(dev, size_add(mux_size, sizeof_priv), GFP_KERNEL);
> > if (!muxc)
> > return NULL;
> > if (sizeof_priv)
>
> The new variable makes it more readable, but beyond that, do you see any
> reason not to just directly compose the calls?
>
You could do that too.
You pointed this out in your other email but the one thing that people
have to be careful of when assigning struct_size() is that the
"mux_size" variable has to be size_t.
The math in submit_create() from drivers/gpu/drm/msm/msm_gem_submit.c
is so terribly unreadable. It works but it's so ugly. Unfortunately,
I'm the person who wrote it.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] i2c: mux: harden i2c_mux_alloc() against integer overflows
2022-09-16 8:20 ` Dan Carpenter
@ 2022-09-16 19:31 ` Wolfram Sang
2022-09-19 6:35 ` Dan Carpenter
0 siblings, 1 reply; 14+ messages in thread
From: Wolfram Sang @ 2022-09-16 19:31 UTC (permalink / raw)
To: Dan Carpenter
Cc: Kees Cook, Peter Rosin, Gustavo A. R. Silva, linux-i2c,
linux-kernel, kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 649 bytes --]
> > The new variable makes it more readable, but beyond that, do you see any
> > reason not to just directly compose the calls?
> >
>
> You could do that too.
>
> You pointed this out in your other email but the one thing that people
> have to be careful of when assigning struct_size() is that the
> "mux_size" variable has to be size_t.
>
> The math in submit_create() from drivers/gpu/drm/msm/msm_gem_submit.c
> is so terribly unreadable. It works but it's so ugly. Unfortunately,
> I'm the person who wrote it.
I can't parse from that if the patch in question is okay or needs a
respin? Could you kindly enlighten me?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] i2c: mux: harden i2c_mux_alloc() against integer overflows
2022-09-16 19:31 ` Wolfram Sang
@ 2022-09-19 6:35 ` Dan Carpenter
0 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2022-09-19 6:35 UTC (permalink / raw)
To: Wolfram Sang, Kees Cook, Peter Rosin, Gustavo A. R. Silva,
linux-i2c, linux-kernel, kernel-janitors
On Fri, Sep 16, 2022 at 08:31:58PM +0100, Wolfram Sang wrote:
>
> > > The new variable makes it more readable, but beyond that, do you see any
> > > reason not to just directly compose the calls?
> > >
> >
> > You could do that too.
> >
> > You pointed this out in your other email but the one thing that people
> > have to be careful of when assigning struct_size() is that the
> > "mux_size" variable has to be size_t.
> >
> > The math in submit_create() from drivers/gpu/drm/msm/msm_gem_submit.c
> > is so terribly unreadable. It works but it's so ugly. Unfortunately,
> > I'm the person who wrote it.
>
> I can't parse from that if the patch in question is okay or needs a
> respin? Could you kindly enlighten me?
>
It doesn't need a respin. We were just discussing related bugs with the
integer overflow safe functions.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] i2c: mux: harden i2c_mux_alloc() against integer overflows
2022-09-15 11:30 [PATCH] i2c: mux: harden i2c_mux_alloc() against integer overflows Dan Carpenter
` (2 preceding siblings ...)
2022-09-16 8:01 ` Kees Cook
@ 2022-09-21 20:13 ` Wolfram Sang
3 siblings, 0 replies; 14+ messages in thread
From: Wolfram Sang @ 2022-09-21 20:13 UTC (permalink / raw)
To: Dan Carpenter
Cc: Peter Rosin, Gustavo A. R. Silva, linux-i2c, linux-kernel,
kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 583 bytes --]
On Thu, Sep 15, 2022 at 02:30:58PM +0300, Dan Carpenter wrote:
> A couple years back we went through the kernel an automatically
> converted size calculations to use struct_size() instead. The
> struct_size() calculation is protected against integer overflows.
>
> However it does not make sense to use the result from struct_size()
> for additional math operations as that would negate any safeness.
>
> Fixes: 1f3b69b6b939 ("i2c: mux: Use struct_size() in devm_kzalloc()")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Applied to for-current, thanks!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread