Re: [PATCH bpf 1/3] bpf: Explicitly check accesses to bpf_sock_addr

[Paul Chaignon <paul.chaignon@gmail.com>]

On Tue, Sep 16, 2025 at 09:44:28PM +0200, Daniel Borkmann wrote:
> On 9/16/25 5:17 PM, Paul Chaignon wrote:
> > Syzkaller found a kernel warning on the following sock_addr program:
> > 
> >      0: r0 = 0
> >      1: r2 = *(u32 *)(r1 +60)
> >      2: exit
> > 
> > which triggers:
> > 
> >      verifier bug: error during ctx access conversion (0)
> > 
> > This is happening because offset 60 in bpf_sock_addr corresponds to an
> > implicit padding of 4 bytes, right after msg_src_ip4. Access to this
> > padding isn't rejected in sock_addr_is_valid_access and it thus later
> > fails to convert the access.
> > 
> > This patch fixes it by explicitly checking the various fields of
> > bpf_sock_addr in sock_addr_is_valid_access.
> > 
> > I checked the other ctx structures and is_valid_access functions and
> > didn't find any other similar cases. Other cases of (properly handled)
> > padding are covered in new tests in a subsequent patch.
> > 
> > Fixes: 1cedee13d25a ("bpf: Hooks for sys_sendmsg")
> > Reported-by: syzbot+136ca59d411f92e821b7@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=136ca59d411f92e821b7
> > Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
> > ---
> >   net/core/filter.c | 8 +++++++-
> >   1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index da391e2b0788..9ac58960e59e 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -9284,13 +9284,19 @@ static bool sock_addr_is_valid_access(int off, int size,
> >   			return false;
> >   		info->reg_type = PTR_TO_SOCKET;
> >   		break;
> > -	default:
> > +	case bpf_ctx_range(struct bpf_sock_addr, user_family):
> > +	case bpf_ctx_range(struct bpf_sock_addr, family):
> > +	case bpf_ctx_range(struct bpf_sock_addr, type):
> > +	case bpf_ctx_range(struct bpf_sock_addr, protocol):
> >   		if (type == BPF_READ) {
> >   			if (size != size_default)
> >   				return false;
> >   		} else {
> >   			return false;
> >   		}
> > +		break;
> 
> Looks good to me, we can probably also simplify this a bit into:

Thanks for the review!

> 
>                 if (type != BPF_READ)
>                         return false;
>                 if (size != size_default)
>                         return false;
>                 break;
> 
> Also, targeting bpf-next tree seems okay here since the verifier
> can gracefully handle and reject such program (and WARN_ONCE is
> only triggered on CONFIG_DEBUG_KERNEL)?

Yep, that makes sense. I did not think this through :')

> 
> > +	default:
> > +		return false;
> >   	}
> >   	return true;
> 
> Thanks,
> Daniel