Re: [PATCH net] rds: tcp: fix uninit-value in __inet_bind

[Charalampos Mitrodimas]

Tabrez Ahmed <tabreztalks@xxxxxxxxx> writes:

> KMSAN reported an uninit-value access in __inet_bind() when binding an RDS TCP socket.
> The uninitialized memory originates from rds_tcp_conn_alloc(), which uses kmem_cache_alloc() to allocate the rds_tcp_connection structure.
>
> The structure is not zero-initialized, leaving random data in its fields.
> When the networking stack later tries to bind the socket using these dirty values, KMSAN flags the uninitialized access.

Most fields in rds_tcp_connection are explicitly initialized after
allocation right? The only field actually read before being written is
t_client_port_group. Could the description be more specific about which
field is problematic?

>
> Fix this by using kmem_cache_zalloc() instead of kmem_cache_alloc() to ensure the structure is zeroed out upon allocation.
>
> Reported-by: syzbot+aae646f09192f72a68dc@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=aae646f09192f72a68dc
> Tested-by: syzbot+aae646f09192f72a68dc@xxxxxxxxxxxxxxxxxxxxxxxxx
> Fixes: 70041088e3b9 ("RDS: Add TCP transport to RDS")

Not sure about this, the field trigger this bug, t_client_port_group,
did not exist in 70041088e3b9. It was introduced in

  a20a6992558f ("net/rds: Encode cp_index in TCP source port")

which added both the field and the code in rds_tcp_conn_path_connect()
that reads it uninitialized:

     if (++tc->t_client_port_group >= port_groups)

Should the Fixes that reference that instead? If I'm correct ofc.

Also, nit, commit message body should not exceed 75 characters.

>
> Signed-off-by: Tabrez Ahmed <tabreztalks@xxxxxxxxx>
> ---
> This is my first patch. Any feedback is appreciated!

Best of luck.

>
>  net/rds/tcp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/rds/tcp.c b/net/rds/tcp.c
> index 45484a93d75f..04f310255692 100644
> --- a/net/rds/tcp.c
> +++ b/net/rds/tcp.c
> @@ -373,7 +373,7 @@ static int rds_tcp_conn_alloc(struct rds_connection *conn, gfp_t gfp)
>  	int ret = 0;
>  
>  	for (i = 0; i < RDS_MPATH_WORKERS; i++) {
> -		tc = kmem_cache_alloc(rds_tcp_conn_slab, gfp);
> +		tc = kmem_cache_zalloc(rds_tcp_conn_slab, gfp);
>  		if (!tc) {
>  			ret = -ENOMEM;
>  			goto fail;