diff mbox series

ipw2x00: Avoid field-overflowing memcpy()

Message ID 20210806200855.2870554-1-keescook@chromium.org (mailing list archive)
State Awaiting Upstream
Delegated to: Netdev Maintainers
Headers show
Series ipw2x00: Avoid field-overflowing memcpy() | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Kees Cook Aug. 6, 2021, 8:08 p.m. UTC 
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.

libipw_read_qos_param_element() copies a struct libipw_info_element
into a struct libipw_qos_information_element, but is actually wanting to
copy into the larger struct libipw_qos_parameter_info (the contents of
ac_params_record[] is later examined). Refactor the routine to perform
centralized checks, and copy the entire contents directly (since the id
and len members match the elementID and length members):

struct libipw_info_element {
        u8 id;
        u8 len;
        u8 data[];
} __packed;

struct libipw_qos_information_element {
        u8 elementID;
        u8 length;
        u8 qui[QOS_OUI_LEN];
        u8 qui_type;
        u8 qui_subtype;
        u8 version;
        u8 ac_info;
} __packed;

struct libipw_qos_parameter_info {
        struct libipw_qos_information_element info_element;
        u8 reserved;
        struct libipw_qos_ac_parameter ac_params_record[QOS_QUEUE_NUM];
} __packed;

Cc: Kalle Valo <kvalo@codeaurora.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: linux-wireless@vger.kernel.org
Cc: netdev@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 .../net/wireless/intel/ipw2x00/libipw_rx.c    | 56 ++++++-------------
 1 file changed, 17 insertions(+), 39 deletions(-)

Comments

Kees Cook Aug. 6, 2021, 10:30 p.m. UTC | #1 
On Fri, Aug 06, 2021 at 01:08:55PM -0700, Kees Cook wrote:
> -	if (element_info == NULL)
> +	if (!element_info || info_element || info_element->len != size - 2)
>  		return -1;
> -	if (info_element == NULL)
> -		return -1;

Gah, a let a typo in. (should be !info_element.) v2 coming...
kernel test robot Aug. 7, 2021, 2:36 a.m. UTC | #2 
Hi Kees,

I love your patch! Perhaps something to improve:

[auto build test WARNING on wireless-drivers-next/master]
[also build test WARNING on wireless-drivers/master kees/for-next/pstore v5.14-rc4 next-20210806]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Kees-Cook/ipw2x00-Avoid-field-overflowing-memcpy/20210807-041024
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git master
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 10.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/8f3acfe1fbe7b1bad6ff871b98209bbbf6581992
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Kees-Cook/ipw2x00-Avoid-field-overflowing-memcpy/20210807-041024
        git checkout 8f3acfe1fbe7b1bad6ff871b98209bbbf6581992
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross ARCH=ia64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In function 'libipw_read_qos_info_element',
       inlined from 'libipw_parse_qos_info_param_IE' at drivers/net/wireless/intel/ipw2x00/libipw_rx.c:1025:7:
>> drivers/net/wireless/intel/ipw2x00/libipw_rx.c:973:2: warning: argument 2 null where non-null expected [-Wnonnull]
     973 |  memcpy(element_info, info_element, size);
         |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   In file included from include/linux/string.h:20,
                    from include/linux/bitmap.h:10,
                    from include/linux/cpumask.h:12,
                    from include/linux/smp.h:13,
                    from include/linux/lockdep.h:14,
                    from include/linux/spinlock.h:59,
                    from include/linux/mmzone.h:8,
                    from include/linux/gfp.h:6,
                    from include/linux/mm.h:10,
                    from include/linux/bvec.h:14,
                    from include/linux/skbuff.h:17,
                    from include/linux/if_arp.h:22,
                    from drivers/net/wireless/intel/ipw2x00/libipw_rx.c:14:
   drivers/net/wireless/intel/ipw2x00/libipw_rx.c: In function 'libipw_parse_qos_info_param_IE':
   arch/ia64/include/asm/string.h:19:14: note: in a call to function 'memcpy' declared here
      19 | extern void *memcpy (void *, const void *, __kernel_size_t);
         |              ^~~~~~


vim +973 drivers/net/wireless/intel/ipw2x00/libipw_rx.c

   960	
   961	/*
   962	 * Parse a QoS information element
   963	 */
   964	static int libipw_read_qos_info_element(
   965				struct libipw_qos_information_element *element_info,
   966				struct libipw_info_element *info_element)
   967	{
   968		size_t size = sizeof(struct libipw_qos_information_element) - 2;
   969	
   970		if (!element_info || info_element || info_element->len != size - 2)
   971			return -1;
   972	
 > 973		memcpy(element_info, info_element, size);
   974		return libipw_verify_qos_info(element_info, QOS_OUI_INFO_SUB_TYPE);
   975	}
   976	
   977	/*
   978	 * Write QoS parameters from the ac parameters.
   979	 */
   980	static void libipw_qos_convert_ac_to_parameters(struct
   981							  libipw_qos_parameter_info
   982							  *param_elm, struct
   983							  libipw_qos_parameters
   984							  *qos_param)
   985	{
   986		int i;
   987		struct libipw_qos_ac_parameter *ac_params;
   988		u32 txop;
   989		u8 cw_min;
   990		u8 cw_max;
   991	
   992		for (i = 0; i < QOS_QUEUE_NUM; i++) {
   993			ac_params = &(param_elm->ac_params_record[i]);
   994	
   995			qos_param->aifs[i] = (ac_params->aci_aifsn) & 0x0F;
   996			qos_param->aifs[i] -= (qos_param->aifs[i] < 2) ? 0 : 2;
   997	
   998			cw_min = ac_params->ecw_min_max & 0x0F;
   999			qos_param->cw_min[i] = cpu_to_le16((1 << cw_min) - 1);
  1000	
  1001			cw_max = (ac_params->ecw_min_max & 0xF0) >> 4;
  1002			qos_param->cw_max[i] = cpu_to_le16((1 << cw_max) - 1);
  1003	
  1004			qos_param->flag[i] =
  1005			    (ac_params->aci_aifsn & 0x10) ? 0x01 : 0x00;
  1006	
  1007			txop = le16_to_cpu(ac_params->tx_op_limit) * 32;
  1008			qos_param->tx_op_limit[i] = cpu_to_le16(txop);
  1009		}
  1010	}
  1011	
  1012	/*
  1013	 * we have a generic data element which it may contain QoS information or
  1014	 * parameters element. check the information element length to decide
  1015	 * which type to read
  1016	 */
  1017	static int libipw_parse_qos_info_param_IE(struct libipw_info_element
  1018						     *info_element,
  1019						     struct libipw_network *network)
  1020	{
  1021		int rc = 0;
  1022		struct libipw_qos_parameters *qos_param = NULL;
  1023		struct libipw_qos_information_element qos_info_element;
  1024	
> 1025		rc = libipw_read_qos_info_element(&qos_info_element, info_element);
  1026	
  1027		if (rc == 0) {
  1028			network->qos_data.param_count = qos_info_element.ac_info & 0x0F;
  1029			network->flags |= NETWORK_HAS_QOS_INFORMATION;
  1030		} else {
  1031			struct libipw_qos_parameter_info param_element;
  1032	
  1033			rc = libipw_read_qos_param_element(&param_element,
  1034							      info_element);
  1035			if (rc == 0) {
  1036				qos_param = &(network->qos_data.parameters);
  1037				libipw_qos_convert_ac_to_parameters(&param_element,
  1038								       qos_param);
  1039				network->flags |= NETWORK_HAS_QOS_PARAMETERS;
  1040				network->qos_data.param_count =
  1041				    param_element.info_element.ac_info & 0x0F;
  1042			}
  1043		}
  1044	
  1045		if (rc == 0) {
  1046			LIBIPW_DEBUG_QOS("QoS is supported\n");
  1047			network->qos_data.supported = 1;
  1048		}
  1049		return rc;
  1050	}
  1051	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

diff --git a/drivers/net/wireless/intel/ipw2x00/libipw_rx.c b/drivers/net/wireless/intel/ipw2x00/libipw_rx.c
index 5a2a723e480b..7cda31e403bd 100644
--- a/drivers/net/wireless/intel/ipw2x00/libipw_rx.c
+++ b/drivers/net/wireless/intel/ipw2x00/libipw_rx.c
@@ -927,7 +927,8 @@  static u8 qos_oui[QOS_OUI_LEN] = { 0x00, 0x50, 0xF2 };
 static int libipw_verify_qos_info(struct libipw_qos_information_element
 				     *info_element, int sub_type)
 {
-
+	if (info_element->elementID != QOS_ELEMENT_ID)
+		return -1;
 	if (info_element->qui_subtype != sub_type)
 		return -1;
 	if (memcmp(info_element->qui, qos_oui, QOS_OUI_LEN))
@@ -943,57 +944,34 @@  static int libipw_verify_qos_info(struct libipw_qos_information_element
 /*
  * Parse a QoS parameter element
  */
-static int libipw_read_qos_param_element(struct libipw_qos_parameter_info
-					    *element_param, struct libipw_info_element
-					    *info_element)
+static int libipw_read_qos_param_element(
+			struct libipw_qos_parameter_info *element_param,
+			struct libipw_info_element *info_element)
 {
-	int ret = 0;
-	u16 size = sizeof(struct libipw_qos_parameter_info) - 2;
+	size_t size = sizeof(*element_param);
 
-	if ((info_element == NULL) || (element_param == NULL))
+	if (!element_param || !info_element || info_element->len != size - 2)
 		return -1;
 
-	if (info_element->id == QOS_ELEMENT_ID && info_element->len == size) {
-		memcpy(element_param->info_element.qui, info_element->data,
-		       info_element->len);
-		element_param->info_element.elementID = info_element->id;
-		element_param->info_element.length = info_element->len;
-	} else
-		ret = -1;
-	if (ret == 0)
-		ret = libipw_verify_qos_info(&element_param->info_element,
-						QOS_OUI_PARAM_SUB_TYPE);
-	return ret;
+	memcpy(element_param, info_element, size);
+	return libipw_verify_qos_info(&element_param->info_element,
+				      QOS_OUI_PARAM_SUB_TYPE);
 }
 
 /*
  * Parse a QoS information element
  */
-static int libipw_read_qos_info_element(struct
-					   libipw_qos_information_element
-					   *element_info, struct libipw_info_element
-					   *info_element)
+static int libipw_read_qos_info_element(
+			struct libipw_qos_information_element *element_info,
+			struct libipw_info_element *info_element)
 {
-	int ret = 0;
-	u16 size = sizeof(struct libipw_qos_information_element) - 2;
+	size_t size = sizeof(struct libipw_qos_information_element) - 2;
 
-	if (element_info == NULL)
+	if (!element_info || info_element || info_element->len != size - 2)
 		return -1;
-	if (info_element == NULL)
-		return -1;
-
-	if ((info_element->id == QOS_ELEMENT_ID) && (info_element->len == size)) {
-		memcpy(element_info->qui, info_element->data,
-		       info_element->len);
-		element_info->elementID = info_element->id;
-		element_info->length = info_element->len;
-	} else
-		ret = -1;
 
-	if (ret == 0)
-		ret = libipw_verify_qos_info(element_info,
-						QOS_OUI_INFO_SUB_TYPE);
-	return ret;
+	memcpy(element_info, info_element, size);
+	return libipw_verify_qos_info(element_info, QOS_OUI_INFO_SUB_TYPE);
 }
 
 /*