[v1] SUNRPC: Increase size of servername string

[PATCH] SUNRPC: Increase size of servername string

Posted by Haowen Bai 4 years, 2 months ago

This patch will fix the warning from smatch:

net/sunrpc/clnt.c:562 rpc_create() error: snprintf() chops off
the last chars of 'sun->sun_path': 108 vs 48

Signed-off-by: Haowen Bai <baihaowen@meizu.com>
---
 net/sunrpc/clnt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index c83fe61..6e0209e 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -526,7 +526,7 @@ struct rpc_clnt *rpc_create(struct rpc_create_args *args)
 		.servername = args->servername,
 		.bc_xprt = args->bc_xprt,
 	};
-	char servername[48];
+	char servername[108];
 	struct rpc_clnt *clnt;
 	int i;
 
-- 
2.7.4

Re: [PATCH] SUNRPC: Increase size of servername string

Posted by NeilBrown 4 years, 2 months ago

On Thu, 24 Mar 2022, Haowen Bai wrote:
> This patch will fix the warning from smatch:
> 
> net/sunrpc/clnt.c:562 rpc_create() error: snprintf() chops off
> the last chars of 'sun->sun_path': 108 vs 48
> 
> Signed-off-by: Haowen Bai <baihaowen@meizu.com>
> ---
>  net/sunrpc/clnt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index c83fe61..6e0209e 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -526,7 +526,7 @@ struct rpc_clnt *rpc_create(struct rpc_create_args *args)
>  		.servername = args->servername,
>  		.bc_xprt = args->bc_xprt,
>  	};
> -	char servername[48];
> +	char servername[108];

It would be much nicer to use UNIX_PATH_MAX

NeilBrown


>  	struct rpc_clnt *clnt;
>  	int i;
>  
> -- 
> 2.7.4
> 
>

RE: [PATCH] SUNRPC: Increase size of servername string

Posted by David Laight 4 years, 2 months ago

From: NeilBrown
> Sent: 25 March 2022 02:07
> 
> On Thu, 24 Mar 2022, Haowen Bai wrote:
> > This patch will fix the warning from smatch:
> >
> > net/sunrpc/clnt.c:562 rpc_create() error: snprintf() chops off
> > the last chars of 'sun->sun_path': 108 vs 48
> >
> > Signed-off-by: Haowen Bai <baihaowen@meizu.com>
> > ---
> >  net/sunrpc/clnt.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> > index c83fe61..6e0209e 100644
> > --- a/net/sunrpc/clnt.c
> > +++ b/net/sunrpc/clnt.c
> > @@ -526,7 +526,7 @@ struct rpc_clnt *rpc_create(struct rpc_create_args *args)
> >  		.servername = args->servername,
> >  		.bc_xprt = args->bc_xprt,
> >  	};
> > -	char servername[48];
> > +	char servername[108];
> 
> It would be much nicer to use UNIX_PATH_MAX

No on-stack....

Given the use:

	if (xprtargs.servername == NULL) {
		struct sockaddr_un *sun =
				(struct sockaddr_un *)args->address;
		struct sockaddr_in *sin =
				(struct sockaddr_in *)args->address;
		struct sockaddr_in6 *sin6 =
				(struct sockaddr_in6 *)args->address;

		servername[0] = '\0';
		switch (args->address->sa_family) {
		case AF_LOCAL:
			snprintf(servername, sizeof(servername), "%s",
				 sun->sun_path);
			break;
		case AF_INET:
			snprintf(servername, sizeof(servername), "%pI4",
				 &sin->sin_addr.s_addr);
			break;
		case AF_INET6:
			snprintf(servername, sizeof(servername), "%pI6",
				 &sin6->sin6_addr);
			break;
		default:
			/* caller wants default server name, but
			 * address family isn't recognized. */
			return ERR_PTR(-EINVAL);
		}
		xprtargs.servername = servername;
	}

It looks like the AF_LOCAL case could be:
		xprtargs.servername = sun->sun_path;
Then the buffer only needs to be big enough for the IPv6 address.
For which 40 is enough.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH] SUNRPC: Increase size of servername string

Posted by David Laight 4 years, 2 months ago

From: David Laight
> Sent: 25 March 2022 06:53
> 
> From: NeilBrown
> > Sent: 25 March 2022 02:07
> >
> > On Thu, 24 Mar 2022, Haowen Bai wrote:
> > > This patch will fix the warning from smatch:
> > >
> > > net/sunrpc/clnt.c:562 rpc_create() error: snprintf() chops off
> > > the last chars of 'sun->sun_path': 108 vs 48
> > >
> > > Signed-off-by: Haowen Bai <baihaowen@meizu.com>
> > > ---
> > >  net/sunrpc/clnt.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> > > index c83fe61..6e0209e 100644
> > > --- a/net/sunrpc/clnt.c
> > > +++ b/net/sunrpc/clnt.c
> > > @@ -526,7 +526,7 @@ struct rpc_clnt *rpc_create(struct rpc_create_args *args)
> > >  		.servername = args->servername,
> > >  		.bc_xprt = args->bc_xprt,
> > >  	};
> > > -	char servername[48];
> > > +	char servername[108];
> >
> > It would be much nicer to use UNIX_PATH_MAX
> 
> Not on-stack....

Ok I looked the constant up - it is 108.
OTOH just looking at the code makes it look like a value
that is much larger - not good on stack.
Even [sizeof sun->sun_path] would probably be better.
But I don't think the copy is needed at all.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)