| Message ID | 20210806200855.2870554-1-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | ipw2x00: Avoid field-overflowing memcpy() | expand |
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...
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(¶m_element,
1034 info_element);
1035 if (rc == 0) {
1036 qos_param = &(network->qos_data.parameters);
1037 libipw_qos_convert_ac_to_parameters(¶m_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 --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); } /*
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(-)