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

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

[Deepanshu Kartikey]

dev->ci_range.vpi_bits and vci_bits can legitimately be ATM_CI_MAX (-1),
a sentinel meaning "no range configured, use maximum" (see
include/uapi/linux/atmdev.h). Some drivers, such as usbatm_atm_init()
in drivers/usb/atm/usbatm.c, set this sentinel and never resolve it to
an actual bit width.

__vcc_connect() uses these fields directly as a shift amount without
checking for the sentinel, so binding a PVC socket on such a device
triggers a negative shift:

  UBSAN: shift-out-of-bounds in net/atm/common.c:381:10
  shift exponent -1 is negative

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

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: syzbot+6665d3db5fef15914802@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=6665d3db5fef15914802
Tested-by: syzbot+6665d3db5fef15914802@syzkaller.appspotmail.com
Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
---
 net/atm/common.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/atm/common.c b/net/atm/common.c
index 81195727fa18..73057bb016f7 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;
 	if (vci > 0 && vci < ATM_NOT_RSV_VCI && !capable(CAP_NET_BIND_SERVICE))
 		return -EPERM;
-- 
2.34.1

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

[Simon Horman]

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.

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

[Eric Dumazet]

On Wed, Aug 26, 2026 at 1:59 PM Simon Horman <horms@kernel.org> wrote:
>
> 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

A simpler fix would be something like

   usb: atm: usbatm: fix invalid ci_range initialization

    syzbot reported a shift-out-of-bounds in __vcc_connect():

     UBSAN: shift-out-of-bounds in net/atm/common.c:382:32
     shift exponent -1 is negative
     CPU: 0 UID: 0 PID: 5987 Comm: syz.0.18 Not tainted syzkaller #0
PREEMPT(full)
     Hardware name: Google Google Compute Engine/Google Compute
Engine, BIOS Google 08/05/2026
     Call Trace:
      <TASK>
      dump_stack_lvl+0xe8/0x150 lib/dump_stack.c:120
      ubsan_epilogue+0xa/0x30 lib/ubsan.c:233
      __ubsan_handle_shift_out_of_bounds+0x36d/0x400 lib/ubsan.c:494
      __vcc_connect+0x14b4/0x19c0 net/atm/common.c:382
      vcc_connect+0x328/0x8f0 net/atm/common.c:498
      pvc_bind+0x272/0x380 net/atm/pvc.c:52
      __sys_bind+0x2e3/0x410 net/socket.c:1976
      __x64_sys_bind+0x7a/0x90 net/socket.c:1979
      ...

    ATM device ci_range fields (vpi_bits and vci_bits) represent the number
    of bits supported for VPI and VCI addressing on the device.
    net/atm/common.c directly uses these fields as bit shift counts:
      vpi >> dev->ci_range.vpi_bits
      vci >> dev->ci_range.vci_bits
      1 << vcc->dev->ci_range.vpi_bits
      1 << vcc->dev->ci_range.vci_bits

    usbatm_atm_init() sets ci_range.vpi_bits and ci_range.vci_bits to
    ATM_CI_MAX (-1), which was defined in <uapi/linux/atmdev.h> as a sentinel
    value for user-space ATM_SETCIRANGE requests, not as a valid bit count.
    Shifting by -1 is undefined behavior and triggers UBSAN warnings.

    ATM UNI cell headers allow up to 8 bits for VPI (0..255) and 16 bits
    for VCI (0..65535).
    Initialize vpi_bits to 8 and vci_bits to 16, as done by solos-pci.

 ...


diff --git a/drivers/usb/atm/usbatm.c b/drivers/usb/atm/usbatm.c
index 9600e1ec099304e465dddb00c36817339f538b3b..7b0c791399eaac9ebe00541a2c41d715f465a24f
100644
--- a/drivers/usb/atm/usbatm.c
+++ b/drivers/usb/atm/usbatm.c
@@ -917,8 +917,8 @@ static int usbatm_atm_init(struct usbatm_data *instance)

        instance->atm_dev = atm_dev;

-       atm_dev->ci_range.vpi_bits = ATM_CI_MAX;
-       atm_dev->ci_range.vci_bits = ATM_CI_MAX;
+       atm_dev->ci_range.vpi_bits = 8;
+       atm_dev->ci_range.vci_bits = 16;
        atm_dev->signal = ATM_PHY_SIG_UNKNOWN;

        /* temp init ATM device, set to 128kbit */

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

[Deepanshu Kartikey]

On Wed, Aug 26, 2026 at 5:34 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Wed, Aug 26, 2026 at 1:59 PM Simon Horman <horms@kernel.org> wrote:
> >
> > 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
>
> A simpler fix would be something like
>
>    usb: atm: usbatm: fix invalid ci_range initialization
>
>     syzbot reported a shift-out-of-bounds in __vcc_connect():
>
>      UBSAN: shift-out-of-bounds in net/atm/common.c:382:32
>      shift exponent -1 is negative
>      CPU: 0 UID: 0 PID: 5987 Comm: syz.0.18 Not tainted syzkaller #0
> PREEMPT(full)
>      Hardware name: Google Google Compute Engine/Google Compute
> Engine, BIOS Google 08/05/2026
>      Call Trace:
>       <TASK>
>       dump_stack_lvl+0xe8/0x150 lib/dump_stack.c:120
>       ubsan_epilogue+0xa/0x30 lib/ubsan.c:233
>       __ubsan_handle_shift_out_of_bounds+0x36d/0x400 lib/ubsan.c:494
>       __vcc_connect+0x14b4/0x19c0 net/atm/common.c:382
>       vcc_connect+0x328/0x8f0 net/atm/common.c:498
>       pvc_bind+0x272/0x380 net/atm/pvc.c:52
>       __sys_bind+0x2e3/0x410 net/socket.c:1976
>       __x64_sys_bind+0x7a/0x90 net/socket.c:1979
>       ...
>
>     ATM device ci_range fields (vpi_bits and vci_bits) represent the number
>     of bits supported for VPI and VCI addressing on the device.
>     net/atm/common.c directly uses these fields as bit shift counts:
>       vpi >> dev->ci_range.vpi_bits
>       vci >> dev->ci_range.vci_bits
>       1 << vcc->dev->ci_range.vpi_bits
>       1 << vcc->dev->ci_range.vci_bits
>
>     usbatm_atm_init() sets ci_range.vpi_bits and ci_range.vci_bits to
>     ATM_CI_MAX (-1), which was defined in <uapi/linux/atmdev.h> as a sentinel
>     value for user-space ATM_SETCIRANGE requests, not as a valid bit count.
>     Shifting by -1 is undefined behavior and triggers UBSAN warnings.
>
>     ATM UNI cell headers allow up to 8 bits for VPI (0..255) and 16 bits
>     for VCI (0..65535).
>     Initialize vpi_bits to 8 and vci_bits to 16, as done by solos-pci.
>
>  ...
>
>
> diff --git a/drivers/usb/atm/usbatm.c b/drivers/usb/atm/usbatm.c
> index 9600e1ec099304e465dddb00c36817339f538b3b..7b0c791399eaac9ebe00541a2c41d715f465a24f
> 100644
> --- a/drivers/usb/atm/usbatm.c
> +++ b/drivers/usb/atm/usbatm.c
> @@ -917,8 +917,8 @@ static int usbatm_atm_init(struct usbatm_data *instance)
>
>         instance->atm_dev = atm_dev;
>
> -       atm_dev->ci_range.vpi_bits = ATM_CI_MAX;
> -       atm_dev->ci_range.vci_bits = ATM_CI_MAX;
> +       atm_dev->ci_range.vpi_bits = 8;
> +       atm_dev->ci_range.vci_bits = 16;
>         atm_dev->signal = ATM_PHY_SIG_UNKNOWN;
>
>         /* temp init ATM device, set to 128kbit */

 Initially, I was thinking the same way. Thanks for reviewing it. I
will send patch v2 shortly

Thanks

Deepanshu