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

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260824024620.23485-1-kartikey406@gmail.com?part=1
---
net: atm: fix shift-out-of-bounds in __vcc_connect()

__vcc_connect() used dev->ci_range.vpi_bits and vci_bits directly as shift
amounts, which is a negative shift when those fields still hold the
ATM_CI_MAX (-1) sentinel.  The patch skips the VPI/VCI range check
entirely when either field equals ATM_CI_MAX.

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

> diff --git a/net/atm/common.c b/net/atm/common.c
> index 81195727fa18b..b38359453555b 100644
> --- a/net/atm/common.c
> +++ b/net/atm/common.c
> @@ -378,8 +378,11 @@ 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 &&
> +	    dev->ci_range.vpi_bits != ATM_CI_MAX &&
>  	    vpi >> dev->ci_range.vpi_bits) || (vci != ATM_VCI_UNSPEC &&
> -	    vci != ATM_VCI_ANY && vci >> dev->ci_range.vci_bits))
> +	    vci != ATM_VCI_ANY &&
> +	    dev->ci_range.vci_bits != ATM_CI_MAX &&
> +	    vci >> dev->ci_range.vci_bits))
>  		return -EINVAL;

[Severity: High]
This isn't a bug introduced by this patch, but does the same negative shift
remain reachable from the function being modified here?  find_ci() is
called by __vcc_connect() a few lines below:

net/atm/common.c:__vcc_connect() {
	...
	if (test_bit(ATM_DF_REMOVED, &dev->flags) ||
	    (error = find_ci(vcc, &vpi, &vci))) {
	...
}

and find_ci() shifts by the same fields with no sentinel check:

net/atm/common.c:find_ci() {
	...
	else if (p >= 1 << vcc->dev->ci_range.vpi_bits)
		p = 0;
	if (*vci != ATM_VCI_ANY)
		c = *vci;
	else if (c < ATM_NOT_RSV_VCI || c >= 1 << vcc->dev->ci_range.vci_bits)
			c = ATM_NOT_RSV_VCI;
	...
		if (*vci == ATM_VCI_ANY) {
			c++;
			if (c >= 1 << vcc->dev->ci_range.vci_bits)
				c = ATM_NOT_RSV_VCI;
		}
	...
			p++;
			if (p >= 1 << vcc->dev->ci_range.vpi_bits)
				p = 0;
	...
}

Note the new guards in __vcc_connect() are skipped when vpi is ATM_VPI_ANY
or vci is ATM_VCI_ANY (both -1 in include/uapi/linux/atm.h), which is
exactly the case that then reaches find_ci().  pvc_bind() passes the
userspace sockaddr_atmpvc values through vcc_connect() unchanged, and
vcc_connect() only rejects !(vpi || vci):

net/atm/pvc.c:pvc_bind() {
	...
	error = vcc_connect(sock, addr->sap_addr.itf, addr->sap_addr.vpi,
			    addr->sap_addr.vci);
	...
}

On a device that leaves the sentinel in place, e.g. usbatm_atm_init():

drivers/usb/atm/usbatm.c:usbatm_atm_init() {
	...
	atm_dev->ci_range.vpi_bits = ATM_CI_MAX;
	atm_dev->ci_range.vci_bits = ATM_CI_MAX;
	...
}

wouldn't a bind() with sap_addr.vpi or sap_addr.vci set to -1 still produce
the same "shift exponent -1 is negative" report, only from find_ci()
instead of __vcc_connect()?  With panic_on_warn set, ubsan_epilogue()
calls check_panic_on_warn("UBSAN"), so this would still be a panic.

There also looks to be a secondary effect: since 1 << -1 folds to INT_MIN
on architectures that mask the shift count, both p >= 1 << vpi_bits and
c >= 1 << vci_bits are always true, so after the first wildcard VCC is
allocated the do/while scan in find_ci() hits old_p == p && old_c == c on
the first iteration and returns -EADDRINUSE.  Does that make a second
automatically-allocated VC impossible on such a device?

Would it be preferable to resolve the sentinel to a real bit width once,
so both __vcc_connect() and find_ci() see a sane value?

[Severity: Medium]
Separately, is skipping the check the intended reading of the sentinel?
The uAPI describes it as the maximum range rather than as unrestricted:

include/uapi/linux/atmdev.h:
#define ATM_CI_MAX      -1              /* use maximum range of VPI/VCI */

struct atm_cirange {
	signed char	vpi_bits;	/* 1..8, ATM_CI_MAX (-1) for maximum */
	signed char	vci_bits;	/* 1..16, ATM_CI_MAX (-1) for maximum */
};

with the maxima defined in include/uapi/linux/atm.h as ATM_MAX_VPI 255 and
ATM_MAX_VCI 65535.  The commit message says:

  "Skip the range check when ci_range.vpi_bits/vci_bits is still
  ATM_CI_MAX, since that value means "unrestricted"."

Should this instead clamp to 8/16 bits (or compare against ATM_MAX_VPI /
ATM_MAX_VCI) and keep the check?

This also appears to change behaviour for negative identifiers other than
the ANY/UNSPEC magic values.  Before the patch, vpi >> -1 folded to
vpi >> 31 on architectures that mask the shift count, which is -1 for a
negative vpi and therefore returned -EINVAL.  After the patch a bind() with
sap_addr.vpi = -5 or sap_addr.vci = -65531 is accepted, since vcc_connect()
only rejects !(vpi || vci) and check_ci() masks vci purely for hashing:

net/atm/common.c:check_ci() {
	struct hlist_head *head = &vcc_hash[vci & (VCC_HTABLE_SIZE - 1)];
	...
}

The raw value then reaches vcc->vpi/vcc->vci and dev->ops->open().
usbatm_atm_open() does not bound them, and usbatm_write_cells() packs them
into the fixed-width cell header without masking:

drivers/usb/atm/usbatm.c:usbatm_write_cells() {
	...
	ptr[0] = vcc->vpi >> 4;
	ptr[1] = (vcc->vpi << 4) | (vcc->vci >> 12);
	ptr[2] = vcc->vci >> 4;
	ptr[3] = vcc->vci << 4;
	...
}

Can this alias distinct VCCs onto the same on-the-wire VPI/VCI, for example
vci = -65531 and vci = 65541 both transmitting on reserved VCI 5, while
the check

	if (vci > 0 && vci < ATM_NOT_RSV_VCI && !capable(CAP_NET_BIND_SERVICE))
		return -EPERM;

is evaluated on the untruncated value and so does not fire?  The duplicate
detection in check_ci() compares the untruncated values too, so it would
not catch the collision either.