Re: [PATCH] net/atm: fix shift-out-of-bounds in __vcc_connect()/find_ci()

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 (2) · Medium: 2 · Low: 0

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

- [Medium] This patch says it fixes the UBSAN report 'shift-out-of-bounds
  in net/atm/common.c:382:32, shift exponent -1 is negative'…
- [Medium] ci_range_bound() (net/atm/common.c:338-343) uses `if (bits <=
  0 || bits > max_bits) return 1 << max_bits;`, which treats bits == 0…

Pre-existing issues:
- [High] find_ci() checks the static VPI cache `p` only against its upper
  bound and never resets it when negative.
- [High] solos-pci's atm_init() (drivers/atm/solos-pci.c:1348) calls
  atm_dev_register() at line 1359.

--

net/atm: fix shift-out-of-bounds in __vcc_connect()/find_ci()

This commit adds a ci_range_bound() helper to net/atm/common.c. The helper
turns ci_range.vpi_bits/vci_bits into an exclusive upper bound. It falls
back to the maximum range for ATM_CI_MAX, or for any other value outside the
documented range. The direct shifts in find_ci() and __vcc_connect() are
replaced with it, and __vcc_connect() gains explicit vpi/vci < 0 checks.

Closes: https://syzkaller.appspot.com/bug?extid=f6ac161ee9699270b8dd

> usbatm_atm_init() has exactly that window: it calls atm_dev_register()
> and only afterwards sets ci_range.vpi_bits/vci_bits. A bind() on
> AF_ATMPVC/AF_ATMSVC racing in during that window sees a vci_bits (or
> vpi_bits) value outside the documented range, and the plain shift is
> undefined behaviour, reported by UBSAN as a negative shift exponent.

[Severity: Medium]
Is this description accurate for the current tree?

Commit a60fd8c6dbaa76 ("usb: atm: usbatm: fix invalid ci_range
initialization") already fixes this UBSAN splat. It has the same "shift
exponent -1 is negative" in __vcc_connect(), reached through
pvc_bind()->vcc_connect(). In that report the -1 was ATM_CI_MAX, which
usbatm_atm_init() stored in ci_range permanently. It did not come from a
registration race.

With that commit applied, usbatm_atm_init() does:

	atm_dev->ci_range.vpi_bits = 8;
	atm_dev->ci_range.vci_bits = 16;

solos-pci atm_init() does the same. Neither driver implements
ATM_SETCIRANGE.

The atm_dev comes from kzalloc_obj() in __alloc_atm_dev(). During the
register-before-assignment window the fields are therefore 0, and shifting
by 0 is well defined. The new comment above ci_range_bound() makes the same
claim, that "the all-zero value the field holds before a driver's probe has
set it" is not a valid shift count.

Is there still a path that reaches a negative or oversized shift count?

The reproduction was run on syzbot's kernel image, which appears to be older
than a60fd8c6dbaa76. The Closes: tag (extid f6ac161ee9699270b8dd) looks like
a duplicate of the report that commit closed (extid 6665d3db5fef15914802).

Should this patch be dropped as a duplicate? Alternatively, it could be
resent as hardening, with a description that references a60fd8c6dbaa76.

> diff --git a/net/atm/common.c b/net/atm/common.c
> index 81195727fa18b..bb6bc26eb5992 100644
> --- a/net/atm/common.c
> +++ b/net/atm/common.c
> @@ -327,6 +327,21 @@ static int check_ci(const struct atm_vcc *vcc, short vpi, int vci)
>  	return 0;
>  }
>  
> +/*
> + * ci_range.vpi_bits/vci_bits are documented as 1..8 / 1..16, with
> + * ATM_CI_MAX (-1) meaning "no restriction". Neither that sentinel nor
> + * the all-zero value the field holds before a driver's probe has set
> + * it (see the register-before-ci_range-is-set race in usbatm_atm_init())
> + * is a valid shift count, so compute the exclusive upper bound instead
> + * of shifting a VPI/VCI by a possibly negative or out-of-range amount.
> + */
> +static int ci_range_bound(signed char bits, int max_bits)
> +{
> +	if (bits <= 0 || bits > max_bits)
> +		return 1 << max_bits;

[Severity: Medium]
Should bits == 0 really be treated as the maximum-range case here?

A width of 0 bits is well defined. 1 << 0 is 1, so only VPI/VCI 0 is valid.

Before this patch, a device with vpi_bits == 0 rejected any explicit
vpi != 0 in __vcc_connect(), because vpi >> 0 is nonzero. find_ci() also
limited its scan to p < 1.

After this patch, __vcc_connect() accepts VPI 0..255 and VCI 0..65535 for
such a device. find_ci() can also auto-assign values in that range. An unset
range now gets the most permissive bound instead of the most restrictive
one.

In this tree, bits == 0 only occurs between atm_dev_register() and the
driver's ci_range assignment. This change therefore lets explicit-CI binds
through while the device is only half initialised (see the solos-pci comment
below). It would also affect any driver that uses 0 bits to mean "VPI 0
only" and indexes tables by VPI/VCI.

Would it be enough to apply the fallback only for bits < 0 (ATM_CI_MAX)? That
avoids the undefined shift and keeps the old behaviour for 0.

> +	return 1 << bits;
> +}
> +
>  static int find_ci(const struct atm_vcc *vcc, short *vpi, int *vci)
>  {
>  	static short p;        /* poor man's per-device cache */
> @@ -342,12 +357,13 @@ static int find_ci(const struct atm_vcc *vcc, short *vpi, int *vci)
>  	/* last scan may have left values out of bounds for current device */
>  	if (*vpi != ATM_VPI_ANY)
>  		p = *vpi;
> -	else if (p >= 1 << vcc->dev->ci_range.vpi_bits)
> +	else if (p >= ci_range_bound(vcc->dev->ci_range.vpi_bits, 8))
>  		p = 0;

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but the
patch rewrites this line. Should the static cache p also be reset when it is
negative?

A bind with vpi = ATM_VPI_UNSPEC (-2) and vci = ATM_VCI_ANY runs p = *vpi,
which leaves the static p at -2. A later ATM_VPI_ANY request, on any device,
only checks p >= bound, so p stays at -2.

The VCI cache has a matching lower-bound check (c < ATM_NOT_RSV_VCI). The
VPI cache does not.

This seems to cause two problems.

First, find_ci() can return *vpi = -2, or -1 after p++, for an ATM_VPI_ANY
request. vcc_connect() sets ATM_VF_PARTIAL based on the caller's vpi:

	if (vpi == ATM_VPI_UNSPEC || vci == ATM_VCI_UNSPEC)
		set_bit(ATM_VF_PARTIAL, &vcc->flags);

The socket is therefore treated as fully connected with a sentinel VPI.
usbatm and solos then write that VPI into cell/packet headers.

Second, once old_p is negative, this exit condition can never be met,
because p++ only wraps back to 0:

	} while (old_p != p || old_c != c);

Take vpi = ATM_VPI_ANY with an explicit vci. If (-2, vci), (-1, vci) and
(0..255, vci) are all occupied, can this loop spin forever under
write_lock_irq(&vcc_sklist_lock)?

This sequence seems to reach it for an unprivileged user with a usbatm
device:

  bind(UNSPEC, ANY)         -> p = -2
  bind(UNSPEC, 100)         -> occupies (-2, 100), p unchanged
  bind(ANY, 100)            -> gets (-1, 100)
  bind(UNSPEC, ANY)         -> p = -2 again
  bind(0..255, 100) x 256   -> explicit, p unchanged
  bind(ANY, 100)            -> p never returns to old_p = -2

usbatm_atm_open() does not range-check vpi. It also sets ATM_VF_ADDR, which
check_ci() needs to detect collisions. The sequence takes about 260 sockets,
each with UBR QoS.

[ ... ]

> @@ -378,8 +394,9 @@ static int __vcc_connect(struct atm_vcc *vcc, struct atm_dev *dev, short vpi,
>  	int error;
>  
>  	if ((vpi != ATM_VPI_UNSPEC && vpi != ATM_VPI_ANY &&
> -	    vpi >> dev->ci_range.vpi_bits) || (vci != ATM_VCI_UNSPEC &&
> -	    vci != ATM_VCI_ANY && vci >> dev->ci_range.vci_bits))
> +	    (vpi < 0 || vpi >= ci_range_bound(dev->ci_range.vpi_bits, 8))) ||
> +	    (vci != ATM_VCI_UNSPEC && vci != ATM_VCI_ANY &&
> +	    (vci < 0 || vci >= ci_range_bound(dev->ci_range.vci_bits, 16))))
>  		return -EINVAL;

[Severity: High]
This isn't a bug introduced by this patch, but the zero-to-maximum fallback
opens a second path to an existing NULL dereference in solos-pci.

drivers/atm/solos-pci.c:atm_init() publishes the device before it sets up
ci_range and dev_data:

	card->atmdev[i] = atm_dev_register("solos-pci", parent, &fpga_ops, -1, NULL);
	...
	card->atmdev[i]->ci_range.vpi_bits = 8;
	card->atmdev[i]->ci_range.vci_bits = 16;
	card->atmdev[i]->dev_data = card;

device_create_file(), sysfs_create_group() and dev_info() run in between.

A concurrent bind(AF_ATMPVC) in that window goes through
pvc_bind()->vcc_connect()->__vcc_connect()->popen(). popen() reads
card = vcc->dev->dev_data, which is still NULL, and calls
fpga_queue(card, ...). fpga_queue() then does:

	spin_lock_irqsave(&card->tx_queue_lock, flags);

Before this patch, wildcard binds already passed the zero-width check, and
find_ci() resolved them to (0, 32). Now ci_range_bound(0, 16) returns 65536,
so explicit requests such as (0, 32) or (5, 100) also get through. The old
vci >> 0 check rejected those.

usbatm_atm_open() returns -ENODEV while dev_data is NULL, but popen() has no
readiness check at all.

Would this need the device to be initialised before atm_dev_register(), or a
synchronised readiness check in popen()? Changing how the bound is computed
does not seem enough on its own.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260927120410.44906-1-ngocthang2710.1999%40gmail.com