Re: [PATCH net] atm: pppoatm: reject duplicate backend assignment

[netdev-bot+sashiko]

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

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

- [High] Incomplete fix: the new `if (atmvcc->user_back) return -EBUSY;`
  guard in pppoatm_assign_vcc() (net/atm/pppoatm.c:399) only rejects a…

Pre-existing issues:
- [Medium] pppoatm_assign_vcc() publishes the backend with two plain,
  unordered stores: `atmvcc->user_back = pvcc;` immediately followed by…

--

atm: pppoatm: reject duplicate backend assignment

This adds a check in pppoatm_assign_vcc() that returns -EBUSY when
atmvcc->user_back is already populated, so a second ATM_SETBACKEND with
ATM_BACKEND_PPP cannot stack a second pppoatm_vcc on the same VCC. The
commit message describes the resulting NULL user_back dereference during
VCC teardown as the bug being fixed.

> diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
> index 6da52d12df68e..93ba22f437bc9 100644
> --- a/net/atm/pppoatm.c
> +++ b/net/atm/pppoatm.c
> @@ -396,6 +396,8 @@ static int pppoatm_assign_vcc(struct atm_vcc *atmvcc, void __user *arg)
>  	if (be.encaps != PPPOATM_ENCAPS_AUTODETECT &&
>  	    be.encaps != PPPOATM_ENCAPS_VC && be.encaps != PPPOATM_ENCAPS_LLC)
>  		return -EINVAL;
> +	if (atmvcc->user_back)
> +		return -EBUSY;

[Severity: High]
Does this guard close the described crash, or only the pppoatm-over-pppoatm
ordering of it?  pppoatm is not the only writer of atmvcc->user_back.
br2684_regvcc() in net/atm/br2684.c writes the same field, and its only
ownership checks are:

net/atm/br2684.c:br2684_regvcc() {
	...
	if (atmvcc->push == NULL) {
		err = -EBADFD;
		goto error;
	}
	if (!list_empty(&brdev->brvccs)) {
		/* Only 1 VCC/dev right now */
		err = -EEXIST;
		goto error;
	}
	...
}

After pppoatm_assign_vcc() has installed pppoatm_push, atmvcc->push is
non-NULL, and a freshly created nas device from ATM_NEWBACKENDIF has an
empty brvccs list, so neither check fires.  br2684_regvcc() then does:

net/atm/br2684.c:br2684_regvcc() {
	...
	atmvcc->user_back = brvcc;
	brvcc->encaps = (enum br2684_encaps)be.encaps;
	brvcc->old_push = atmvcc->push;
	...
}

which overwrites the pppoatm private pointer and chains pppoatm_push as
old_push.

On close, vcc_destroy_socket() calls vcc->push(vcc, NULL) -> br2684_push()
-> br2684_close_vcc():

net/atm/br2684.c:br2684_close_vcc() {
	...
	brvcc->atmvcc->user_back = NULL;	/* what about vcc->recvq ??? */
	brvcc->atmvcc->release_cb = brvcc->old_release_cb;
	brvcc->old_push(brvcc->atmvcc, NULL);	/* pass on the bad news */
	...
}

so pppoatm_push() runs with user_back already NULL and dereferences it:

net/atm/pppoatm.c:pppoatm_push() {
	...
		module = pvcc->old_owner;
		pppoatm_unassign_vcc(atmvcc);
	...
}

Is that not the same NULL dereference the commit message says is removed,
just with the two ATM_SETBACKEND calls in the opposite order?

While both backends are stacked, can this also corrupt memory?  br2684_pop()
calls brvcc->old_pop, which is pppoatm_pop():

net/atm/pppoatm.c:pppoatm_pop() {
	struct pppoatm_vcc *pvcc = atmvcc_to_pvcc(atmvcc);

	pvcc->old_pop(atmvcc, skb);
	atomic_dec(&pvcc->inflight);
	...
}

Here user_back points at a struct br2684_vcc, so pvcc->old_pop is an
indirect call through br2684_vcc::old_push and atomic_dec(&pvcc->inflight)
writes into br2684_vcc::old_owner.  The same type confusion applies to
br2684_release_cb() -> pppoatm_release_cb().  The first pvcc also stays
registered via ppp_register_channel() with a live tasklet, and its
__module_get(THIS_MODULE) is never balanced, since teardown now runs through
br2684_push().

Regarding this part of the commit message:

    do_vcc_ioctl() holds ioctl_mutex while invoking backend handlers, so
    concurrent backend assignments cannot race past this check.

Is serialization the relevant property here?  do_vcc_ioctl() offers
ATM_SETBACKEND to every registered backend handler with no core-level
"backend already assigned" rejection:

net/atm/ioctl.c:do_vcc_ioctl() {
	...
	list_for_each(pos, &ioctl_list) {
		struct atm_ioctl *ic = list_entry(pos, struct atm_ioctl, list);
		if (try_module_get(ic->owner)) {
			error = ic->ioctl(sock, cmd, arg);
	...
}

so the other writer of user_back runs under the same mutex without any such
guard.  Would it make sense to add the same rejection to br2684_regvcc(), or
to do the check once in the shared ATM_SETBACKEND dispatch instead?

>  	pvcc = kzalloc_obj(*pvcc);
>  	if (pvcc == NULL)
>  		return -ENOMEM;

[Severity: Medium]
This isn't a bug introduced by this patch, but since the same field is
involved: is the publication of user_back ordered against the publication of
the push callback?  Further down in pppoatm_assign_vcc() the two stores are
plain and adjacent:

net/atm/pppoatm.c:pppoatm_assign_vcc() {
	...
	atmvcc->user_back = pvcc;
	atmvcc->push = pppoatm_push;
	atmvcc->pop = pppoatm_pop;
	...
}

ATM_SETBACKEND requires sock->state == SS_CONNECTED, and the driver receive
path calls vcc->push(vcc, skb) from softirq context without any lock shared
with do_vcc_ioctl(), which holds only ioctl_mutex.  If the store to
atmvcc->push becomes visible first, pppoatm_push() reads user_back as NULL
and dereferences it with no NULL check:

net/atm/pppoatm.c:pppoatm_push() {
	struct pppoatm_vcc *pvcc = atmvcc_to_pvcc(atmvcc);
	pr_debug("\n");
	if (skb == NULL) {
	...
		module = pvcc->old_owner;
	...
	switch (pvcc->encaps) {
	...
}

The sibling backend orders the same sequence explicitly:

net/atm/br2684.c:br2684_regvcc() {
	...
	brvcc->old_owner = atmvcc->owner;
	barrier();
	atmvcc->push = br2684_push;
	...
}

Would smp_store_release(&atmvcc->push, pppoatm_push) after user_back is set
be appropriate here?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902040320.4007552-1-v3rdant.xiang%40gmail.com