Re: [PATCH] net: ipv4: ipconfig: fix OOB read in DHCP option parsing

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider.

Critical: 0 · High: 2 · Medium: 1 · Low: 3

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix is incomplete.
- [High] ic_do_bootp_ext() (net/ipv4/ipconfig.c) reads fixed-size
  payloads whatever the option's declared length: `memcpy(&ic_netmask,…
- [Medium] The patch also changes the post-advance check in the DHCP loop
  of ic_bootp_recv() from `if (ext >= end) break;` to `if (ext > end)…
- [Low] The commit message describes the bug inaccurately.
- [Low] The patch indents the whole DHCP option loop in ic_bootp_recv()
  by one extra tab.
- [Low] The patch presents itself as a fix for an out-of-bounds read
  triggerable by a rogue DHCP server, but it has no `Fixes:` tag.

--

net: ipv4: ipconfig: fix OOB read in DHCP option parsing

This patch adds an ext >= end guard to the DHCP-only option loop in
ic_bootp_recv(). The guard stops the length byte from being read when the
option code is the last byte of the packet. It also relaxes the
post-advance check from ext >= end to ext > end and re-indents the loop.

> The DHCP option parser in ic_bootp_recv() advances the option
> pointer by the option length and only checks that the new pointer
> has not reached the end of the packet.  The length byte and the
> option payload (opt[1], opt[2]) are then read without verifying
> that they lie within the buffer, so a malformed reply from a
> rogue DHCP server can trigger an out-of-bounds read.

[Severity: Low]
Is this description of the payload reads accurate? In the original loop
the post-advance check ran before the switch:

	ext += *ext + 1;
	if (ext >= end)
		break;
	switch (*opt) {

So reaching the switch already meant opt + 2 + opt[1] < end. That covers
opt[2] for option 53 and opt + 2 .. opt + 5 for option 54.

The only out-of-bounds access was the 1-byte read of *ext when ext == end
after ext++. Whatever that byte held, ext then went past end and the loop
broke, so the value was never used.

The payload reads that really lack a check are in ic_do_bootp_ext(), which
this patch does not touch (see below).

> Validate the option length against the remaining bytes before
> consuming the option, and skip options that do not fit.

The new code uses break, not continue, so it stops parsing rather than
skipping the option. Could the message describe that?

> Reviewed-by: Guanglei Zhu <zhugl3@xiaopeng.com>
> Signed-off-by: Liu Chao <liuc63@xiaopeng.com>

[Severity: Low]
The patch is presented as a fix for an out-of-bounds read that a rogue
DHCP server can trigger, but it has no Fixes: tag. The parser lines it
changes come from the initial git import. Should it carry this tag?

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")

> diff --git a/net/ipv4/ipconfig.c b/net/ipv4/ipconfig.c
> index 1b8585404a41d..e2c8bcec615a2 100644
> --- a/net/ipv4/ipconfig.c
> +++ b/net/ipv4/ipconfig.c
> @@ -1083,25 +1083,27 @@ static int __init ic_bootp_recv(struct sk_buff *skb, struct net_device *dev, str
>  			__be32 server_id = NONE;
>  			int mt = 0;
>  
> -			ext = &b->exten[4];
> -			while (ext < end && *ext != 0xff) {

[ ... ]

> +				ext = &b->exten[4];
> +				while (ext < end && *ext != 0xff) {

[Severity: Low]
This isn't a bug, but the loop is now one tab deeper than the other
statements in the same block: server_id and mt above it, and the
pr_debug() and switch (mt) after it.

The functional change is two lines, but the diff rewrites the whole loop.
That makes the real change and git blame harder to follow. Could the
original indentation be kept?

> +					u8 *opt = ext++;
> +					if (*opt == 0)	/* Padding */
> +						continue;
> +					if (ext >= end)
> +						break;

[Severity: High]
This guard is added only to the DHCP-only loop. The generic option loop
later in ic_bootp_recv() walks the same buffer and still has the same
pattern:

net/ipv4/ipconfig.c:ic_bootp_recv() {
    ...
	ext = &b->exten[4];
	while (ext < end && *ext != 0xff) {
		u8 *opt = ext++;
		if (*opt == 0)	/* Padding */
			continue;
		ext += *ext + 1;
		if (ext < end)
			ic_do_bootp_ext(opt);
	}
    ...
}

Say the last byte of the datagram is a non-zero option code other than
0xff. Then ext++ makes ext == end, so wouldn't the following *ext still
read *end, the same access this patch removes from the DHCP loop?

The break here only ends the first scan and does not reject the packet.
The same malformed suffix therefore reaches the second loop. That loop
also runs for plain ip=bootp replies when IC_USE_DHCP is not set.

The earlier pskb_may_pull(skb, skb->len) means the byte is still inside
the skb head allocation. The practical effect is the same as for the loop
being fixed.

The commit message says the change ensures "all reads stay within the
received packet". Should the second loop get the same guard?

> +					ext += *ext + 1;
> +					if (ext > end)
> +						break;

[Severity: Medium]
This comparison changed from ext >= end to ext > end, and the commit
message does not mention it.

Take an option that ends exactly at the end of the datagram, with no 0xff
terminator. Before, it was ignored: a trailing message type option left
mt == 0 and the reply was dropped as "Urque". Now it is parsed, and mt or
server_id are taken from it.

The commit message says the patch "keeps the existing parsing logic
intact". Is this change intended?

The generic loop below still uses the strict rule:

		ext += *ext + 1;
		if (ext < end)
			ic_do_bootp_ext(opt);

So the two parsers of one option list now disagree about whether the
final option is valid. Could the change be called out in the message, or
could the two loops be made consistent?

> +					switch (*opt) {
> +					case 53:	/* Message type */
> +						if (opt[1])
> +							mt = opt[2];
> +						break;
> +					case 54:	/* Server ID (IP address) */
> +						if (opt[1] >= 4)
> +							memcpy(&server_id, opt + 2, 4);
> +						break;

[Severity: High]
Option 54 checks opt[1] >= 4 before its 4-byte copy. The handlers in
ic_do_bootp_ext(), which the generic loop calls, copy fixed sizes without
checking the option length:

net/ipv4/ipconfig.c:ic_do_bootp_ext() {
    ...
	case 1:		/* Subnet mask */
		if (ic_netmask == NONE)
			memcpy(&ic_netmask, ext+1, 4);
		break;
	case 3:		/* Default gateway */
		if (ic_gateway == NONE)
			memcpy(&ic_gateway, ext+1, 4);
		break;
    ...
	case 26:	/* Interface MTU */
		memcpy(&mtu, ext+1, sizeof(mtu));
    ...
}

The caller only checks opt + 2 + opt[1] < end. Consider a subnet mask
option of length 0 ("01 00") whose length byte is at end - 2, followed by
one more byte. The check passes and memcpy reads end - 1 .. end + 2. Can
this copy three bytes from past the received datagram into ic_netmask?

Option 3 does the same with ic_gateway. Option 26 reads one byte past end
into ic_dev_mtu.

Those values are then applied to the interface and printed to the
console. Even inside the packet, a short option makes the handler read
bytes that belong to the next option.

This patch is meant to close out-of-bounds reads in DHCP option parsing
from a rogue server. Should cases 1, 3 and 26 check the option length the
way option 54 does?

> +					}
>  				}
> -			}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261016041152.2294843-1-liuc63%40xiaopeng.com