netdev - Re: [PATCH] bpf: fix reference count leak in bpf_prog_test_run

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87o6n42mfs.fsf@toke.dk>
Date: Thu, 08 Jan 2026 15:44:55 +0100
From: Toke Høiland-Jørgensen <toke@...hat.com>
To: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>, Alexei Starovoitov
 <ast@...nel.org>, John Fastabend <john.fastabend@...il.com>, Lorenzo
 Bianconi <lorenzo@...nel.org>, bpf <bpf@...r.kernel.org>, Network
 Development <netdev@...r.kernel.org>
Subject: Re: [PATCH] bpf: fix reference count leak in bpf_prog_test_run_xdp()

Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp> writes:

> On 2026/01/08 23:01, Toke Høiland-Jørgensen wrote:
>> Hmm, this will end up call bpf_ctx_finish() in the error path, which I'm
>> not sure we want?
>
> Excuse me, but I don't think bpf_ctx_finish() will be called, for
>
> +out_put_dev:
>  	/* We convert the xdp_buff back to an xdp_md before checking the return
>  	 * code so the reference count of any held netdevice will be decremented
>  	 * even if the test run failed.
>  	 */
>  	xdp_convert_buff_to_md(&xdp, ctx);
>  	if (ret) // <== ret was set to non-0 value immediately before the "goto out_put_dev;" line.
>  		goto out;

Oh, right; I think my brain just pattern matched on "if (ret) right
after a function call" and assumed there was an assignment to ret there
as well :D

Okay, not the clearest code flow, but not sure there's a good way to
make it clearer without quite a bit of refactoring.

>  
>  	size = xdp.data_end - xdp.data_meta + sinfo->xdp_frags_size;
>  	ret = bpf_test_finish(kattr, uattr, xdp.data_meta, sinfo, size, sinfo->xdp_frags_size,
>  			      retval, duration);
>  	if (!ret)
>  		ret = bpf_ctx_finish(kattr, uattr, ctx,
>  				     sizeof(struct xdp_md));
>
>> 
>> Could we just move the xdp_convert_md_to_buff() call to after the frags
>> have been copied? Not sure there's technically any dependency there,
>> even though it does look a little off?
>
> Unless
>
> 	xdp_md->data = xdp->data - xdp->data_meta;
> 	xdp_md->data_end = xdp->data_end - xdp->data_meta;
>
> in xdp_convert_buff_to_md() lines do something bad for the error path,
> I think this change will be safe.

Yeah, sure, this should be fine.

Reviewed-by: Toke Høiland-Jørgensen <toke@...hat.com>