octeon_ep: __octep_oq_process_rx() can overflow skb_shinfo()->frags[]

[Maoyi Xie]

Hi,

I think __octep_oq_process_rx() in
drivers/net/ethernet/marvell/octeon_ep/octep_rx.c can overflow
skb_shinfo()->frags[] when the device reports a large packet length. I would
appreciate it if you could let me know whether you agree.

The same overflow has been fixed in several RX paths over the last few
months. The closest is another NIC driver, atlantic, in commit 5ffcb7b890f6
("net: atlantic: fix fragment overflow handling in RX path"). t7xx got the
same fix in f0813bcd2d9d, and phonet got it on the usb-net and gadget sides
in 600dc40554dc and c088d5dd2fff. octeon_ep has the same skb_add_rx_frag()
per chunk loop and I do not think it was covered.

For a packet larger than max_single_buffer_size it adds one fragment per
buffer_size chunk.

	data_len = buff_info->len - oq->max_single_buffer_size;
	while (data_len) {
		...
		skb_add_rx_frag(skb, shinfo->nr_frags,
				buff_info->page, 0,
				buff_info->len, buff_info->len);
		octep_oq_next_pkt(oq, buff_info, &read_idx, &desc_used);
	}

There is no nr_frags < MAX_SKB_FRAGS check. buff_info->len comes from the
device response header (be64_to_cpu(resp_hw->length)). data_len is a u16, and
buffer_size defaults to OCTEP_OQ_BUF_SIZE, about 3776 on 4K pages. A data_len
near 65535 gives about 18 fragments. That is one more than the default
MAX_SKB_FRAGS of 17, so the last skb_add_rx_frag() writes frags[17] past the
array.

I reproduced the write on 7.1-rc7 by replaying that loop with
frags[MAX_SKB_FRAGS] placed on an unmapped page.

  BUG: unable to handle page fault for address: ...
  #PF: error_code(0x0002) - not-present page
  RIP: ... (the skb_add_rx_frag loop), CR2 = the guard page

The fix I had in mind bounds nr_frags and drops the excess, like the atlantic
and t7xx fixes did. octep_vf_rx.c has the same loop and would need the same
change. Does this look like a real overflow to you, and is bounding nr_frags
the right fix? I am happy to send a proper patch with the reproducer once you
confirm.

Kaixuan Li and I found this together.

Thanks,
Maoyi
https://maoyixie.com/