Re: [PATCH] i2c: mux: harden i2c_mux_alloc() against integer overflows

Re: [PATCH] i2c: mux: harden i2c_mux_alloc() against integer overflows

[Kees Cook]

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

Re: [PATCH] i2c: mux: harden i2c_mux_alloc() against integer overflows

[Dan Carpenter]

[-- 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'

Re: [PATCH] i2c: mux: harden i2c_mux_alloc() against integer overflows

[Kees Cook]

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

Re: [PATCH] i2c: mux: harden i2c_mux_alloc() against integer overflows

[Dan Carpenter]

[-- 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);

}

Re: [PATCH] i2c: mux: harden i2c_mux_alloc() against integer overflows

[Kees Cook]

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