[PATCH net,stable] net: cdc_ncm: correct overhead in delayed_ndp_size

[PATCH net,stable] net: cdc_ncm: correct overhead in delayed_ndp_size

[Jouni Seppänen]

From: Jouni K. Seppänen <jks@iki.fi>

Aligning to tx_ndp_modulus is not sufficient because the next align
call can be cdc_ncm_align_tail, which can add up to ctx->tx_modulus +
ctx->tx_remainder - 1 bytes. This used to lead to occasional crashes
on a Huawei 909s-120 LTE module as follows:

- the condition marked /* if there is a remaining skb [...] */ is true
  so the swaps happen
- skb_out is set from ctx->tx_curr_skb
- skb_out->len is exactly 0x3f52
- ctx->tx_curr_size is 0x4000 and delayed_ndp_size is 0xac
  (note that the sum of skb_out->len and delayed_ndp_size is 0x3ffe)
- the for loop over n is executed once
- the cdc_ncm_align_tail call marked /* align beginning of next frame */
  increases skb_out->len to 0x3f56 (the sum is now 0x4002)
- the condition marked /* check if we had enough room left [...] */ is
  false so we break out of the loop
- the condition marked /* If requested, put NDP at end of frame. */ is
  true so the NDP is written into skb_out
- now skb_out->len is 0x4002, so padding_count is minus two interpreted
  as an unsigned number, which is used as the length argument to memset,
  leading to a crash with various symptoms but usually including

> Call Trace:
>  <IRQ>
>  cdc_ncm_fill_tx_frame+0x83a/0x970 [cdc_ncm]
>  cdc_mbim_tx_fixup+0x1d9/0x240 [cdc_mbim]
>  usbnet_start_xmit+0x5d/0x720 [usbnet]

The cdc_ncm_align_tail call first aligns on a ctx->tx_modulus
boundary (adding at most ctx->tx_modulus-1 bytes), then adds
ctx->tx_remainder bytes. Alternatively, the next alignment call can
occur in cdc_ncm_ndp16 or cdc_ncm_ndp32, in which case at most
ctx->tx_ndp_modulus-1 bytes are added.

A similar problem has occurred before, and the code is nontrivial to
reason about, so add a guard before the crashing call. By that time it
is too late to prevent any memory corruption (we'll have written past
the end of the buffer already) but we can at least try to get a warning
written into an on-disk log by avoiding the hard crash caused by padding
past the buffer with a huge number of zeros.

Signed-off-by: Jouni K. Seppänen <jks@iki.fi>
Fixes: 4a0e3e989d66 ("cdc_ncm: Add support for moving NDP to end of NCM frame")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=209407
---
 drivers/net/usb/cdc_ncm.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index e04f588538cc..59f0711b1b63 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -1199,7 +1199,9 @@ cdc_ncm_fill_tx_frame(struct usbnet *dev, struct sk_buff *skb, __le32 sign)
 	 * accordingly. Otherwise, we should check here.
 	 */
 	if (ctx->drvflags & CDC_NCM_FLAG_NDP_TO_END)
-		delayed_ndp_size = ALIGN(ctx->max_ndp_size, ctx->tx_ndp_modulus);
+		delayed_ndp_size = ctx->max_ndp_size +
+			max(ctx->tx_ndp_modulus,
+			    ctx->tx_modulus + ctx->tx_remainder) - 1;
 	else
 		delayed_ndp_size = 0;
 
@@ -1410,7 +1412,8 @@ cdc_ncm_fill_tx_frame(struct usbnet *dev, struct sk_buff *skb, __le32 sign)
 	if (!(dev->driver_info->flags & FLAG_SEND_ZLP) &&
 	    skb_out->len > ctx->min_tx_pkt) {
 		padding_count = ctx->tx_curr_size - skb_out->len;
-		skb_put_zero(skb_out, padding_count);
+		if (!WARN_ON(padding_count > ctx->tx_curr_size))
+			skb_put_zero(skb_out, padding_count);
 	} else if (skb_out->len < ctx->tx_curr_size &&
 		   (skb_out->len % dev->maxpacket) == 0) {
 		skb_put_u8(skb_out, 0);	/* force short packet */
-- 
2.20.1

Re: [PATCH net,stable] net: cdc_ncm: correct overhead in delayed_ndp_size

[kernel test robot]

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

Hi "Jouni,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.11-rc1 next-20201223]
[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/Jouni-Sepp-nen/net-cdc_ncm-correct-overhead-in-delayed_ndp_size/20210103-224538
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 3516bd729358a2a9b090c1905bd2a3fa926e24c6
config: x86_64-randconfig-a003-20210103 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 7af6a134508cd1c7f75c6e3441ce436f220f30a4)
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
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/3d8cc665ef1cf4705135a5a96893a6fdc6dcd398
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jouni-Sepp-nen/net-cdc_ncm-correct-overhead-in-delayed_ndp_size/20210103-224538
        git checkout 3d8cc665ef1cf4705135a5a96893a6fdc6dcd398
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

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 >>):

>> drivers/net/usb/cdc_ncm.c:1203:4: warning: comparison of distinct pointer types ('typeof (ctx->tx_ndp_modulus) *' (aka 'unsigned short *') and 'typeof (ctx->tx_modulus + ctx->tx_remainder) *' (aka 'int *')) [-Wcompare-distinct-pointer-types]
                           max(ctx->tx_ndp_modulus,
                           ^~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/minmax.h:58:19: note: expanded from macro 'max'
   #define max(x, y)       __careful_cmp(x, y, >)
                           ^~~~~~~~~~~~~~~~~~~~~~
   include/linux/minmax.h:42:24: note: expanded from macro '__careful_cmp'
           __builtin_choose_expr(__safe_cmp(x, y), \
                                 ^~~~~~~~~~~~~~~~
   include/linux/minmax.h:32:4: note: expanded from macro '__safe_cmp'
                   (__typecheck(x, y) && __no_side_effects(x, y))
                    ^~~~~~~~~~~~~~~~~
   include/linux/minmax.h:18:28: note: expanded from macro '__typecheck'
           (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
                      ~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~
   1 warning generated.


vim +1203 drivers/net/usb/cdc_ncm.c

  1179	
  1180	struct sk_buff *
  1181	cdc_ncm_fill_tx_frame(struct usbnet *dev, struct sk_buff *skb, __le32 sign)
  1182	{
  1183		struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
  1184		union {
  1185			struct usb_cdc_ncm_nth16 *nth16;
  1186			struct usb_cdc_ncm_nth32 *nth32;
  1187		} nth;
  1188		union {
  1189			struct usb_cdc_ncm_ndp16 *ndp16;
  1190			struct usb_cdc_ncm_ndp32 *ndp32;
  1191		} ndp;
  1192		struct sk_buff *skb_out;
  1193		u16 n = 0, index, ndplen;
  1194		u8 ready2send = 0;
  1195		u32 delayed_ndp_size;
  1196		size_t padding_count;
  1197	
  1198		/* When our NDP gets written in cdc_ncm_ndp(), then skb_out->len gets updated
  1199		 * accordingly. Otherwise, we should check here.
  1200		 */
  1201		if (ctx->drvflags & CDC_NCM_FLAG_NDP_TO_END)
  1202			delayed_ndp_size = ctx->max_ndp_size +
> 1203				max(ctx->tx_ndp_modulus,
  1204				    ctx->tx_modulus + ctx->tx_remainder) - 1;
  1205		else
  1206			delayed_ndp_size = 0;
  1207	
  1208		/* if there is a remaining skb, it gets priority */
  1209		if (skb != NULL) {
  1210			swap(skb, ctx->tx_rem_skb);
  1211			swap(sign, ctx->tx_rem_sign);
  1212		} else {
  1213			ready2send = 1;
  1214		}
  1215	
  1216		/* check if we are resuming an OUT skb */
  1217		skb_out = ctx->tx_curr_skb;
  1218	
  1219		/* allocate a new OUT skb */
  1220		if (!skb_out) {
  1221			if (ctx->tx_low_mem_val == 0) {
  1222				ctx->tx_curr_size = ctx->tx_max;
  1223				skb_out = alloc_skb(ctx->tx_curr_size, GFP_ATOMIC);
  1224				/* If the memory allocation fails we will wait longer
  1225				 * each time before attempting another full size
  1226				 * allocation again to not overload the system
  1227				 * further.
  1228				 */
  1229				if (skb_out == NULL) {
  1230					ctx->tx_low_mem_max_cnt = min(ctx->tx_low_mem_max_cnt + 1,
  1231								      (unsigned)CDC_NCM_LOW_MEM_MAX_CNT);
  1232					ctx->tx_low_mem_val = ctx->tx_low_mem_max_cnt;
  1233				}
  1234			}
  1235			if (skb_out == NULL) {
  1236				/* See if a very small allocation is possible.
  1237				 * We will send this packet immediately and hope
  1238				 * that there is more memory available later.
  1239				 */
  1240				if (skb)
  1241					ctx->tx_curr_size = max(skb->len,
  1242						(u32)USB_CDC_NCM_NTB_MIN_OUT_SIZE);
  1243				else
  1244					ctx->tx_curr_size = USB_CDC_NCM_NTB_MIN_OUT_SIZE;
  1245				skb_out = alloc_skb(ctx->tx_curr_size, GFP_ATOMIC);
  1246	
  1247				/* No allocation possible so we will abort */
  1248				if (skb_out == NULL) {
  1249					if (skb != NULL) {
  1250						dev_kfree_skb_any(skb);
  1251						dev->net->stats.tx_dropped++;
  1252					}
  1253					goto exit_no_skb;
  1254				}
  1255				ctx->tx_low_mem_val--;
  1256			}
  1257			if (ctx->is_ndp16) {
  1258				/* fill out the initial 16-bit NTB header */
  1259				nth.nth16 = skb_put_zero(skb_out, sizeof(struct usb_cdc_ncm_nth16));
  1260				nth.nth16->dwSignature = cpu_to_le32(USB_CDC_NCM_NTH16_SIGN);
  1261				nth.nth16->wHeaderLength = cpu_to_le16(sizeof(struct usb_cdc_ncm_nth16));
  1262				nth.nth16->wSequence = cpu_to_le16(ctx->tx_seq++);
  1263			} else {
  1264				/* fill out the initial 32-bit NTB header */
  1265				nth.nth32 = skb_put_zero(skb_out, sizeof(struct usb_cdc_ncm_nth32));
  1266				nth.nth32->dwSignature = cpu_to_le32(USB_CDC_NCM_NTH32_SIGN);
  1267				nth.nth32->wHeaderLength = cpu_to_le16(sizeof(struct usb_cdc_ncm_nth32));
  1268				nth.nth32->wSequence = cpu_to_le16(ctx->tx_seq++);
  1269			}
  1270	
  1271			/* count total number of frames in this NTB */
  1272			ctx->tx_curr_frame_num = 0;
  1273	
  1274			/* recent payload counter for this skb_out */
  1275			ctx->tx_curr_frame_payload = 0;
  1276		}
  1277	
  1278		for (n = ctx->tx_curr_frame_num; n < ctx->tx_max_datagrams; n++) {
  1279			/* send any remaining skb first */
  1280			if (skb == NULL) {
  1281				skb = ctx->tx_rem_skb;
  1282				sign = ctx->tx_rem_sign;
  1283				ctx->tx_rem_skb = NULL;
  1284	
  1285				/* check for end of skb */
  1286				if (skb == NULL)
  1287					break;
  1288			}
  1289	
  1290			/* get the appropriate NDP for this skb */
  1291			if (ctx->is_ndp16)
  1292				ndp.ndp16 = cdc_ncm_ndp16(ctx, skb_out, sign, skb->len + ctx->tx_modulus + ctx->tx_remainder);
  1293			else
  1294				ndp.ndp32 = cdc_ncm_ndp32(ctx, skb_out, sign, skb->len + ctx->tx_modulus + ctx->tx_remainder);
  1295	
  1296			/* align beginning of next frame */
  1297			cdc_ncm_align_tail(skb_out,  ctx->tx_modulus, ctx->tx_remainder, ctx->tx_curr_size);
  1298	
  1299			/* check if we had enough room left for both NDP and frame */
  1300			if ((ctx->is_ndp16 && !ndp.ndp16) || (!ctx->is_ndp16 && !ndp.ndp32) ||
  1301			    skb_out->len + skb->len + delayed_ndp_size > ctx->tx_curr_size) {
  1302				if (n == 0) {
  1303					/* won't fit, MTU problem? */
  1304					dev_kfree_skb_any(skb);
  1305					skb = NULL;
  1306					dev->net->stats.tx_dropped++;
  1307				} else {
  1308					/* no room for skb - store for later */
  1309					if (ctx->tx_rem_skb != NULL) {
  1310						dev_kfree_skb_any(ctx->tx_rem_skb);
  1311						dev->net->stats.tx_dropped++;
  1312					}
  1313					ctx->tx_rem_skb = skb;
  1314					ctx->tx_rem_sign = sign;
  1315					skb = NULL;
  1316					ready2send = 1;
  1317					ctx->tx_reason_ntb_full++;	/* count reason for transmitting */
  1318				}
  1319				break;
  1320			}
  1321	
  1322			/* calculate frame number within this NDP */
  1323			if (ctx->is_ndp16) {
  1324				ndplen = le16_to_cpu(ndp.ndp16->wLength);
  1325				index = (ndplen - sizeof(struct usb_cdc_ncm_ndp16)) / sizeof(struct usb_cdc_ncm_dpe16) - 1;
  1326	
  1327				/* OK, add this skb */
  1328				ndp.ndp16->dpe16[index].wDatagramLength = cpu_to_le16(skb->len);
  1329				ndp.ndp16->dpe16[index].wDatagramIndex = cpu_to_le16(skb_out->len);
  1330				ndp.ndp16->wLength = cpu_to_le16(ndplen + sizeof(struct usb_cdc_ncm_dpe16));
  1331			} else {
  1332				ndplen = le16_to_cpu(ndp.ndp32->wLength);
  1333				index = (ndplen - sizeof(struct usb_cdc_ncm_ndp32)) / sizeof(struct usb_cdc_ncm_dpe32) - 1;
  1334	
  1335				ndp.ndp32->dpe32[index].dwDatagramLength = cpu_to_le32(skb->len);
  1336				ndp.ndp32->dpe32[index].dwDatagramIndex = cpu_to_le32(skb_out->len);
  1337				ndp.ndp32->wLength = cpu_to_le16(ndplen + sizeof(struct usb_cdc_ncm_dpe32));
  1338			}
  1339			skb_put_data(skb_out, skb->data, skb->len);
  1340			ctx->tx_curr_frame_payload += skb->len;	/* count real tx payload data */
  1341			dev_kfree_skb_any(skb);
  1342			skb = NULL;
  1343	
  1344			/* send now if this NDP is full */
  1345			if (index >= CDC_NCM_DPT_DATAGRAMS_MAX) {
  1346				ready2send = 1;
  1347				ctx->tx_reason_ndp_full++;	/* count reason for transmitting */
  1348				break;
  1349			}
  1350		}
  1351	
  1352		/* free up any dangling skb */
  1353		if (skb != NULL) {
  1354			dev_kfree_skb_any(skb);
  1355			skb = NULL;
  1356			dev->net->stats.tx_dropped++;
  1357		}
  1358	
  1359		ctx->tx_curr_frame_num = n;
  1360	
  1361		if (n == 0) {
  1362			/* wait for more frames */
  1363			/* push variables */
  1364			ctx->tx_curr_skb = skb_out;
  1365			goto exit_no_skb;
  1366	
  1367		} else if ((n < ctx->tx_max_datagrams) && (ready2send == 0) && (ctx->timer_interval > 0)) {
  1368			/* wait for more frames */
  1369			/* push variables */
  1370			ctx->tx_curr_skb = skb_out;
  1371			/* set the pending count */
  1372			if (n < CDC_NCM_RESTART_TIMER_DATAGRAM_CNT)
  1373				ctx->tx_timer_pending = CDC_NCM_TIMER_PENDING_CNT;
  1374			goto exit_no_skb;
  1375	
  1376		} else {
  1377			if (n == ctx->tx_max_datagrams)
  1378				ctx->tx_reason_max_datagram++;	/* count reason for transmitting */
  1379			/* frame goes out */
  1380			/* variables will be reset at next call */
  1381		}
  1382	
  1383		/* If requested, put NDP at end of frame. */
  1384		if (ctx->drvflags & CDC_NCM_FLAG_NDP_TO_END) {
  1385			if (ctx->is_ndp16) {
  1386				nth.nth16 = (struct usb_cdc_ncm_nth16 *)skb_out->data;
  1387				cdc_ncm_align_tail(skb_out, ctx->tx_ndp_modulus, 0, ctx->tx_curr_size - ctx->max_ndp_size);
  1388				nth.nth16->wNdpIndex = cpu_to_le16(skb_out->len);
  1389				skb_put_data(skb_out, ctx->delayed_ndp16, ctx->max_ndp_size);
  1390	
  1391				/* Zero out delayed NDP - signature checking will naturally fail. */
  1392				ndp.ndp16 = memset(ctx->delayed_ndp16, 0, ctx->max_ndp_size);
  1393			} else {
  1394				nth.nth32 = (struct usb_cdc_ncm_nth32 *)skb_out->data;
  1395				cdc_ncm_align_tail(skb_out, ctx->tx_ndp_modulus, 0, ctx->tx_curr_size - ctx->max_ndp_size);
  1396				nth.nth32->dwNdpIndex = cpu_to_le32(skb_out->len);
  1397				skb_put_data(skb_out, ctx->delayed_ndp32, ctx->max_ndp_size);
  1398	
  1399				ndp.ndp32 = memset(ctx->delayed_ndp32, 0, ctx->max_ndp_size);
  1400			}
  1401		}
  1402	
  1403		/* If collected data size is less or equal ctx->min_tx_pkt
  1404		 * bytes, we send buffers as it is. If we get more data, it
  1405		 * would be more efficient for USB HS mobile device with DMA
  1406		 * engine to receive a full size NTB, than canceling DMA
  1407		 * transfer and receiving a short packet.
  1408		 *
  1409		 * This optimization support is pointless if we end up sending
  1410		 * a ZLP after full sized NTBs.
  1411		 */
  1412		if (!(dev->driver_info->flags & FLAG_SEND_ZLP) &&
  1413		    skb_out->len > ctx->min_tx_pkt) {
  1414			padding_count = ctx->tx_curr_size - skb_out->len;
  1415			if (!WARN_ON(padding_count > ctx->tx_curr_size))
  1416				skb_put_zero(skb_out, padding_count);
  1417		} else if (skb_out->len < ctx->tx_curr_size &&
  1418			   (skb_out->len % dev->maxpacket) == 0) {
  1419			skb_put_u8(skb_out, 0);	/* force short packet */
  1420		}
  1421	
  1422		/* set final frame length */
  1423		if (ctx->is_ndp16) {
  1424			nth.nth16 = (struct usb_cdc_ncm_nth16 *)skb_out->data;
  1425			nth.nth16->wBlockLength = cpu_to_le16(skb_out->len);
  1426		} else {
  1427			nth.nth32 = (struct usb_cdc_ncm_nth32 *)skb_out->data;
  1428			nth.nth32->dwBlockLength = cpu_to_le32(skb_out->len);
  1429		}
  1430	
  1431		/* return skb */
  1432		ctx->tx_curr_skb = NULL;
  1433	
  1434		/* keep private stats: framing overhead and number of NTBs */
  1435		ctx->tx_overhead += skb_out->len - ctx->tx_curr_frame_payload;
  1436		ctx->tx_ntbs++;
  1437	
  1438		/* usbnet will count all the framing overhead by default.
  1439		 * Adjust the stats so that the tx_bytes counter show real
  1440		 * payload data instead.
  1441		 */
  1442		usbnet_set_skb_tx_stats(skb_out, n,
  1443					(long)ctx->tx_curr_frame_payload - skb_out->len);
  1444	
  1445		return skb_out;
  1446	
  1447	exit_no_skb:
  1448		/* Start timer, if there is a remaining non-empty skb */
  1449		if (ctx->tx_curr_skb != NULL && n > 0)
  1450			cdc_ncm_tx_timeout_start(ctx);
  1451		return NULL;
  1452	}
  1453	EXPORT_SYMBOL_GPL(cdc_ncm_fill_tx_frame);
  1454	

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 39905 bytes --]

[PATCH net,stable v2] net: cdc_ncm: correct overhead in delayed_ndp_size

[Jouni Seppänen]

From: Jouni K. Seppänen <jks@iki.fi>

Aligning to tx_ndp_modulus is not sufficient because the next align
call can be cdc_ncm_align_tail, which can add up to ctx->tx_modulus +
ctx->tx_remainder - 1 bytes. This used to lead to occasional crashes
on a Huawei 909s-120 LTE module as follows:

- the condition marked /* if there is a remaining skb [...] */ is true
  so the swaps happen
- skb_out is set from ctx->tx_curr_skb
- skb_out->len is exactly 0x3f52
- ctx->tx_curr_size is 0x4000 and delayed_ndp_size is 0xac
  (note that the sum of skb_out->len and delayed_ndp_size is 0x3ffe)
- the for loop over n is executed once
- the cdc_ncm_align_tail call marked /* align beginning of next frame */
  increases skb_out->len to 0x3f56 (the sum is now 0x4002)
- the condition marked /* check if we had enough room left [...] */ is
  false so we break out of the loop
- the condition marked /* If requested, put NDP at end of frame. */ is
  true so the NDP is written into skb_out
- now skb_out->len is 0x4002, so padding_count is minus two interpreted
  as an unsigned number, which is used as the length argument to memset,
  leading to a crash with various symptoms but usually including

> Call Trace:
>  <IRQ>
>  cdc_ncm_fill_tx_frame+0x83a/0x970 [cdc_ncm]
>  cdc_mbim_tx_fixup+0x1d9/0x240 [cdc_mbim]
>  usbnet_start_xmit+0x5d/0x720 [usbnet]

The cdc_ncm_align_tail call first aligns on a ctx->tx_modulus
boundary (adding at most ctx->tx_modulus-1 bytes), then adds
ctx->tx_remainder bytes. Alternatively, the next alignment call can
occur in cdc_ncm_ndp16 or cdc_ncm_ndp32, in which case at most
ctx->tx_ndp_modulus-1 bytes are added.

A similar problem has occurred before, and the code is nontrivial to
reason about, so add a guard before the crashing call. By that time it
is too late to prevent any memory corruption (we'll have written past
the end of the buffer already) but we can at least try to get a warning
written into an on-disk log by avoiding the hard crash caused by padding
past the buffer with a huge number of zeros.

Signed-off-by: Jouni K. Seppänen <jks@iki.fi>
Fixes: 4a0e3e989d66 ("cdc_ncm: Add support for moving NDP to end of NCM frame")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=209407
Reported-by: kernel test robot <lkp@intel.com>
---
v2: cast arguments to max() to the same type because integer promotion can
    result in different types; reported by the kernel test robot

 drivers/net/usb/cdc_ncm.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index e04f588538cc..4d8a1f50190c 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -1199,7 +1199,9 @@ cdc_ncm_fill_tx_frame(struct usbnet *dev, struct sk_buff *skb, __le32 sign)
 	 * accordingly. Otherwise, we should check here.
 	 */
 	if (ctx->drvflags & CDC_NCM_FLAG_NDP_TO_END)
-		delayed_ndp_size = ALIGN(ctx->max_ndp_size, ctx->tx_ndp_modulus);
+		delayed_ndp_size = ctx->max_ndp_size +
+			max((u32)ctx->tx_ndp_modulus,
+			    (u32)ctx->tx_modulus + ctx->tx_remainder) - 1;
 	else
 		delayed_ndp_size = 0;

@@ -1410,7 +1412,8 @@ cdc_ncm_fill_tx_frame(struct usbnet *dev, struct sk_buff *skb, __le32 sign)
 	if (!(dev->driver_info->flags & FLAG_SEND_ZLP) &&
 	    skb_out->len > ctx->min_tx_pkt) {
 		padding_count = ctx->tx_curr_size - skb_out->len;
-		skb_put_zero(skb_out, padding_count);
+		if (!WARN_ON(padding_count > ctx->tx_curr_size))
+			skb_put_zero(skb_out, padding_count);
 	} else if (skb_out->len < ctx->tx_curr_size &&
 		   (skb_out->len % dev->maxpacket) == 0) {
 		skb_put_u8(skb_out, 0);	/* force short packet */
--
2.20.1

Re: [PATCH net,stable v2] net: cdc_ncm: correct overhead in delayed_ndp_size

[Jakub Kicinski]

On Sun,  3 Jan 2021 22:23:09 +0200 Jouni Seppänen wrote:
>  	if (ctx->drvflags & CDC_NCM_FLAG_NDP_TO_END)
> -		delayed_ndp_size = ALIGN(ctx->max_ndp_size, ctx->tx_ndp_modulus);
> +		delayed_ndp_size = ctx->max_ndp_size +
> +			max((u32)ctx->tx_ndp_modulus,
> +			    (u32)ctx->tx_modulus + ctx->tx_remainder) - 1;

Let's use max_t, like Bjorn suggested.

[PATCH net,stable v3] net: cdc_ncm: correct overhead in delayed_ndp_size

[Jouni Seppänen]

From: Jouni K. Seppänen <jks@iki.fi>

Aligning to tx_ndp_modulus is not sufficient because the next align
call can be cdc_ncm_align_tail, which can add up to ctx->tx_modulus +
ctx->tx_remainder - 1 bytes. This used to lead to occasional crashes
on a Huawei 909s-120 LTE module as follows:

- the condition marked /* if there is a remaining skb [...] */ is true
  so the swaps happen
- skb_out is set from ctx->tx_curr_skb
- skb_out->len is exactly 0x3f52
- ctx->tx_curr_size is 0x4000 and delayed_ndp_size is 0xac
  (note that the sum of skb_out->len and delayed_ndp_size is 0x3ffe)
- the for loop over n is executed once
- the cdc_ncm_align_tail call marked /* align beginning of next frame */
  increases skb_out->len to 0x3f56 (the sum is now 0x4002)
- the condition marked /* check if we had enough room left [...] */ is
  false so we break out of the loop
- the condition marked /* If requested, put NDP at end of frame. */ is
  true so the NDP is written into skb_out
- now skb_out->len is 0x4002, so padding_count is minus two interpreted
  as an unsigned number, which is used as the length argument to memset,
  leading to a crash with various symptoms but usually including

> Call Trace:
>  <IRQ>
>  cdc_ncm_fill_tx_frame+0x83a/0x970 [cdc_ncm]
>  cdc_mbim_tx_fixup+0x1d9/0x240 [cdc_mbim]
>  usbnet_start_xmit+0x5d/0x720 [usbnet]

The cdc_ncm_align_tail call first aligns on a ctx->tx_modulus
boundary (adding at most ctx->tx_modulus-1 bytes), then adds
ctx->tx_remainder bytes. Alternatively, the next alignment call can
occur in cdc_ncm_ndp16 or cdc_ncm_ndp32, in which case at most
ctx->tx_ndp_modulus-1 bytes are added.

A similar problem has occurred before, and the code is nontrivial to
reason about, so add a guard before the crashing call. By that time it
is too late to prevent any memory corruption (we'll have written past
the end of the buffer already) but we can at least try to get a warning
written into an on-disk log by avoiding the hard crash caused by padding
past the buffer with a huge number of zeros.

Signed-off-by: Jouni K. Seppänen <jks@iki.fi>
Fixes: 4a0e3e989d66 ("cdc_ncm: Add support for moving NDP to end of NCM frame")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=209407
Reported-by: kernel test robot <lkp@intel.com>
Reviewed-by: Bjørn Mork <bjorn@mork.no>
---
v2: cast arguments to max() to same type, because otherwise integer
    promotion can result in different types
v3: use max_t

 drivers/net/usb/cdc_ncm.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index e04f588538cc..d1346c50d237 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -1199,7 +1199,10 @@ cdc_ncm_fill_tx_frame(struct usbnet *dev, struct sk_buff *skb, __le32 sign)
 	 * accordingly. Otherwise, we should check here.
 	 */
 	if (ctx->drvflags & CDC_NCM_FLAG_NDP_TO_END)
-		delayed_ndp_size = ALIGN(ctx->max_ndp_size, ctx->tx_ndp_modulus);
+		delayed_ndp_size = ctx->max_ndp_size +
+			max_t(u32,
+			      ctx->tx_ndp_modulus,
+			      ctx->tx_modulus + ctx->tx_remainder) - 1;
 	else
 		delayed_ndp_size = 0;
 
@@ -1410,7 +1413,8 @@ cdc_ncm_fill_tx_frame(struct usbnet *dev, struct sk_buff *skb, __le32 sign)
 	if (!(dev->driver_info->flags & FLAG_SEND_ZLP) &&
 	    skb_out->len > ctx->min_tx_pkt) {
 		padding_count = ctx->tx_curr_size - skb_out->len;
-		skb_put_zero(skb_out, padding_count);
+		if (!WARN_ON(padding_count > ctx->tx_curr_size))
+			skb_put_zero(skb_out, padding_count);
 	} else if (skb_out->len < ctx->tx_curr_size &&
 		   (skb_out->len % dev->maxpacket) == 0) {
 		skb_put_u8(skb_out, 0);	/* force short packet */
-- 
2.20.1

Re: [PATCH net,stable v3] net: cdc_ncm: correct overhead in delayed_ndp_size

[David Miller]

From: Jouni Seppänen <jks@iki.fi>
Date: Tue,  5 Jan 2021 06:52:49 +0200

> From: Jouni K. Seppänen <jks@iki.fi>
> 
> Aligning to tx_ndp_modulus is not sufficient because the next align
> call can be cdc_ncm_align_tail, which can add up to ctx->tx_modulus +
> ctx->tx_remainder - 1 bytes. This used to lead to occasional crashes
> on a Huawei 909s-120 LTE module as follows:
> 
> - the condition marked /* if there is a remaining skb [...] */ is true
>   so the swaps happen
> - skb_out is set from ctx->tx_curr_skb
> - skb_out->len is exactly 0x3f52
> - ctx->tx_curr_size is 0x4000 and delayed_ndp_size is 0xac
>   (note that the sum of skb_out->len and delayed_ndp_size is 0x3ffe)
> - the for loop over n is executed once
> - the cdc_ncm_align_tail call marked /* align beginning of next frame */
>   increases skb_out->len to 0x3f56 (the sum is now 0x4002)
> - the condition marked /* check if we had enough room left [...] */ is
>   false so we break out of the loop
> - the condition marked /* If requested, put NDP at end of frame. */ is
>   true so the NDP is written into skb_out
> - now skb_out->len is 0x4002, so padding_count is minus two interpreted
>   as an unsigned number, which is used as the length argument to memset,
>   leading to a crash with various symptoms but usually including
> 
>> Call Trace:
>>  <IRQ>
>>  cdc_ncm_fill_tx_frame+0x83a/0x970 [cdc_ncm]
>>  cdc_mbim_tx_fixup+0x1d9/0x240 [cdc_mbim]
>>  usbnet_start_xmit+0x5d/0x720 [usbnet]
> 
> The cdc_ncm_align_tail call first aligns on a ctx->tx_modulus
> boundary (adding at most ctx->tx_modulus-1 bytes), then adds
> ctx->tx_remainder bytes. Alternatively, the next alignment call can
> occur in cdc_ncm_ndp16 or cdc_ncm_ndp32, in which case at most
> ctx->tx_ndp_modulus-1 bytes are added.
> 
> A similar problem has occurred before, and the code is nontrivial to
> reason about, so add a guard before the crashing call. By that time it
> is too late to prevent any memory corruption (we'll have written past
> the end of the buffer already) but we can at least try to get a warning
> written into an on-disk log by avoiding the hard crash caused by padding
> past the buffer with a huge number of zeros.
> 
> Signed-off-by: Jouni K. Seppänen <jks@iki.fi>
> Fixes: 4a0e3e989d66 ("cdc_ncm: Add support for moving NDP to end of NCM frame")
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=209407
> Reported-by: kernel test robot <lkp@intel.com>
> Reviewed-by: Bjørn Mork <bjorn@mork.no>

Applied and queued up for -stable, thanks.

Re: [PATCH net,stable] net: cdc_ncm: correct overhead in delayed_ndp_size

[Bjørn Mork]

Jouni Seppänen <jks@iki.fi> writes:

> +		delayed_ndp_size = ctx->max_ndp_size +
> +			max(ctx->tx_ndp_modulus,
> +			    ctx->tx_modulus + ctx->tx_remainder) - 1;

You'll probably have to use something like

  max_t(u32, ctx->tx_ndp_modulus, ctx->tx_modulus + ctx->tx_remainder)
  
here as the test robot already said.  Sorry for not seeing that earlier.
Otherwise this looks very good to me. The bug is real and severe, and
your patch appears to be the proper fix for it.

Thanks a lot for figuring this out and taking the time to fixup this
rather messy piece of code.

Reviewed-by: Bjørn Mork <bjorn@mork.no>