From: Jouni K. Seppänen <[email protected]>
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 <[email protected]>
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
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 <[email protected]>
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/[email protected]
From: Jouni K. Seppänen <[email protected]>
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 <[email protected]>
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 <[email protected]>
---
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
Jouni Seppänen <[email protected]> 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 <[email protected]>
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.
From: Jouni K. Seppänen <[email protected]>
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 <[email protected]>
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 <[email protected]>
Reviewed-by: Bjørn Mork <[email protected]>
---
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
From: Jouni Sepp?nen <[email protected]>
Date: Tue, 5 Jan 2021 06:52:49 +0200
> From: Jouni K. Sepp?nen <[email protected]>
>
> 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 <[email protected]>
> 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 <[email protected]>
> Reviewed-by: Bj?rn Mork <[email protected]>
Applied and queued up for -stable, thanks.