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: <172730054387.17050.12447592116066223771@noble.neil.brown.name>
Date: Thu, 26 Sep 2024 07:42:23 +1000
From: "NeilBrown" <neilb@...e.de>
To: "Mirsad Todorovac" <mtodorovac69@...il.com>
Cc: linux-nfs@...r.kernel.org, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, "Chuck Lever" <chuck.lever@...cle.com>,
 "Jeff Layton" <jlayton@...nel.org>, "Olga Kornievskaia" <okorniev@...hat.com>,
 "Dai Ngo" <Dai.Ngo@...cle.com>, "Tom Talpey" <tom@...pey.com>,
 "Trond Myklebust" <trondmy@...nel.org>, "Anna Schumaker" <anna@...nel.org>,
 "David S. Miller" <davem@...emloft.net>, "Eric Dumazet" <edumazet@...gle.com>,
 "Jakub Kicinski" <kuba@...nel.org>, "Paolo Abeni" <pabeni@...hat.com>
Subject: Re: [PATCH v1 1/1] SUNRPC: Make enough room in servername[] for
 AF_UNIX addresses

On Thu, 26 Sep 2024, Mirsad Todorovac wrote:
> On 9/24/24 12:43, NeilBrown wrote:
> > On Tue, 24 Sep 2024, Mirsad Todorovac wrote:
> >> Hi, Neil,
> >>
> >> Apparently I was duplicating work.
> >>
> >> However, using
> >>
> >> 	char servername[UNIX_PATH_MAX];
> >>
> >> has some advantages when compared to hard-coded integer?
> >>
> >> Correct me if I'm wrong.
> > 
> > I think you are wrong.  I agree that 48 is a poor choice.  I think that
> > UNIX_PATH_MAX is a poor choice too.  The "servername" string is used for
> > things other than a UNIX socket path.
> > Did you read all of the thread that I provided a link for?  I suggest a
> > more meaningful number in one of the later messages.
> 
> I see. Thanks for the tip. However, if UNIX_PATH_MAX ever changes in the
> future, the decl
> 
>     char servername[108];
> 
> might be missed when fixing all the changes caused by the change of the
> macro definition? Am I wrong again?

Realistically UNIX_PATH_MAX is never going to change, and if it did that
would not affect the correctness of this code.

> 
> Making it logically depend on the system limits might save some headache
> in the future, perhaps.

Unlikely.  Did you look to see what the failure mode is here?
servername is only ever used in log messages.  Truncating names in log
message at 8 bytes can certainly be annoying.  Truncating at 48 bytes is
much less of a problem.


> 
> If really the biggest string that will be copied there is: "/var/run/rpcbind.sock",
> you are then right - stack space is precious commodity, and allocating
> via kmalloc() might preempt the caller thread.

It might.  But the string is always passed to xprt_create_transport()
which always calls kstrdup() on it.  So maybe that doesn't matter.  (As
I said, understanding the big picture is important).

> 
> However, you got to this five weeks earlier - but the patch did not
> propagate to the main vanilla torvalds tree.

Actually it has.

Commit 9090a7f78623 ("SUNRPC: Fix -Wformat-truncation warning")

$ git show --format=fuller  9090a7f78623 | grep CommitDate
CommitDate: Mon Sep 23 15:03:13 2024 -0400

Linus merged it 
Commit 684a64bf32b6 ("Merge tag 'nfs-for-6.12-1' of git://git.linux-nfs.org/projects/anna/linux-nfs")
Date:   Tue Sep 24 15:44:18 2024 -0700

That patch used RPC_MAXNETNAMELEN which is the least-ugly simple fix.

Thanks for your interest in improving Linux.

NeilBrown

> 
> Best regards,
> Mirsad Todorovac
> 
> > But I really think that the problem here is the warning.  The servername
> > array is ALWAYS big enough for any string that will actually be copied
> > into it.  gcc isn't clever enough to detect that, but it tries to be
> > clever enough to tell you that even though you used snprintf so you know
> > that the string might in theory overflow, it decides to warn you about
> > something you already know.
> > 
> > i.e.  if you want to fix this, I would rather you complain the the
> > compiler writers.  Or maybe suggest a #pragma to silence the warning in
> > this case.  Or maybe examine all of the code instead of the one line
> > that triggers the warning and see if there is a better approach to
> > providing the functionality that is being provided here.
> > 
> > NeilBrown
> > 
> >>
> >> Best regards,
> >> Mirsad Todorovac
> >>
> >> On 9/23/24 23:24, NeilBrown wrote:
> >>> On Tue, 24 Sep 2024, Mirsad Todorovac wrote:
> >>>> GCC 13.2.0 reported with W=1 build option the following warning:
> >>>
> >>> See
> >>>   https://lore.kernel.org/all/20240814093853.48657-1-kunwu.chan@linux.dev/
> >>>
> >>> I don't think anyone really cares about this one.
> >>>
> >>> NeilBrown
> >>>
> >>>
> >>>>
> >>>> net/sunrpc/clnt.c: In function ‘rpc_create’:
> >>>> net/sunrpc/clnt.c:582:75: warning: ‘%s’ directive output may be truncated writing up to 107 bytes into \
> >>>> 					a region of size 48 [-Wformat-truncation=]
> >>>>   582 |                                 snprintf(servername, sizeof(servername), "%s",
> >>>>       |                                                                           ^~
> >>>> net/sunrpc/clnt.c:582:33: note: ‘snprintf’ output between 1 and 108 bytes into a destination of size 48
> >>>>   582 |                                 snprintf(servername, sizeof(servername), "%s",
> >>>>       |                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>>>   583 |                                          sun->sun_path);
> >>>>       |                                          ~~~~~~~~~~~~~~
> >>>>
> >>>>    548         };
> >>>>  → 549         char servername[48];
> >>>>    550         struct rpc_clnt *clnt;
> >>>>    551         int i;
> >>>>    552
> >>>>    553         if (args->bc_xprt) {
> >>>>    554                 WARN_ON_ONCE(!(args->protocol & XPRT_TRANSPORT_BC));
> >>>>    555                 xprt = args->bc_xprt->xpt_bc_xprt;
> >>>>    556                 if (xprt) {
> >>>>    557                         xprt_get(xprt);
> >>>>    558                         return rpc_create_xprt(args, xprt);
> >>>>    559                 }
> >>>>    560         }
> >>>>    561
> >>>>    562         if (args->flags & RPC_CLNT_CREATE_INFINITE_SLOTS)
> >>>>    563                 xprtargs.flags |= XPRT_CREATE_INFINITE_SLOTS;
> >>>>    564         if (args->flags & RPC_CLNT_CREATE_NO_IDLE_TIMEOUT)
> >>>>    565                 xprtargs.flags |= XPRT_CREATE_NO_IDLE_TIMEOUT;
> >>>>    566         /*
> >>>>    567          * If the caller chooses not to specify a hostname, whip
> >>>>    568          * up a string representation of the passed-in address.
> >>>>    569          */
> >>>>    570         if (xprtargs.servername == NULL) {
> >>>>    571                 struct sockaddr_un *sun =
> >>>>    572                                 (struct sockaddr_un *)args->address;
> >>>>    573                 struct sockaddr_in *sin =
> >>>>    574                                 (struct sockaddr_in *)args->address;
> >>>>    575                 struct sockaddr_in6 *sin6 =
> >>>>    576                                 (struct sockaddr_in6 *)args->address;
> >>>>    577
> >>>>    578                 servername[0] = '\0';
> >>>>    579                 switch (args->address->sa_family) {
> >>>>  → 580                 case AF_LOCAL:
> >>>>  → 581                         if (sun->sun_path[0])
> >>>>  → 582                                 snprintf(servername, sizeof(servername), "%s",
> >>>>  → 583                                          sun->sun_path);
> >>>>  → 584                         else
> >>>>  → 585                                 snprintf(servername, sizeof(servername), "@%s",
> >>>>  → 586                                          sun->sun_path+1);
> >>>>  → 587                         break;
> >>>>    588                 case AF_INET:
> >>>>    589                         snprintf(servername, sizeof(servername), "%pI4",
> >>>>    590                                  &sin->sin_addr.s_addr);
> >>>>    591                         break;
> >>>>    592                 case AF_INET6:
> >>>>    593                         snprintf(servername, sizeof(servername), "%pI6",
> >>>>    594                                  &sin6->sin6_addr);
> >>>>    595                         break;
> >>>>    596                 default:
> >>>>    597                         /* caller wants default server name, but
> >>>>    598                          * address family isn't recognized. */
> >>>>    599                         return ERR_PTR(-EINVAL);
> >>>>    600                 }
> >>>>    601                 xprtargs.servername = servername;
> >>>>    602         }
> >>>>    603
> >>>>    604         xprt = xprt_create_transport(&xprtargs);
> >>>>    605         if (IS_ERR(xprt))
> >>>>    606                 return (struct rpc_clnt *)xprt;
> >>>>
> >>>> After the address family AF_LOCAL was added in the commit 176e21ee2ec89, the old hard-coded
> >>>> size for servername of char servername[48] no longer fits. The maximum AF_UNIX address size
> >>>> has now grown to UNIX_PATH_MAX defined as 108 in "include/uapi/linux/un.h" .
> >>>>
> >>>> The lines 580-587 were added later, addressing the leading zero byte '\0', but did not fix
> >>>> the hard-coded servername limit.
> >>>>
> >>>> The AF_UNIX address was truncated to 47 bytes + terminating null byte. This patch will fix the
> >>>> servername in AF_UNIX family to the maximum permitted by the system:
> >>>>
> >>>>    548         };
> >>>>  → 549         char servername[UNIX_PATH_MAX];
> >>>>    550         struct rpc_clnt *clnt;
> >>>>
> >>>> Fixes: 4388ce05fa38b ("SUNRPC: support abstract unix socket addresses")
> >>>> Fixes: 510deb0d7035d ("SUNRPC: rpc_create() default hostname should support AF_INET6 addresses")
> >>>> Fixes: 176e21ee2ec89 ("SUNRPC: Support for RPC over AF_LOCAL transports")
> >>>> Cc: Neil Brown <neilb@...e.de>
> >>>> Cc: Chuck Lever <chuck.lever@...cle.com>
> >>>> Cc: Trond Myklebust <trondmy@...nel.org>
> >>>> Cc: Anna Schumaker <anna@...nel.org>
> >>>> Cc: Jeff Layton <jlayton@...nel.org>
> >>>> Cc: Olga Kornievskaia <okorniev@...hat.com>
> >>>> Cc: Dai Ngo <Dai.Ngo@...cle.com>
> >>>> Cc: Tom Talpey <tom@...pey.com>
> >>>> Cc: "David S. Miller" <davem@...emloft.net>
> >>>> Cc: Eric Dumazet <edumazet@...gle.com>
> >>>> Cc: Jakub Kicinski <kuba@...nel.org>
> >>>> Cc: Paolo Abeni <pabeni@...hat.com>
> >>>> Cc: linux-nfs@...r.kernel.org
> >>>> Cc: netdev@...r.kernel.org
> >>>> Cc: linux-kernel@...r.kernel.org
> >>>> Signed-off-by: Mirsad Todorovac <mtodorovac69@...il.com>
> >>>> ---
> >>>>  v1:
> >>>> 	initial version.
> >>>>
> >>>>  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 09f29a95f2bc..67099719893e 100644
> >>>> --- a/net/sunrpc/clnt.c
> >>>> +++ b/net/sunrpc/clnt.c
> >>>> @@ -546,7 +546,7 @@ struct rpc_clnt *rpc_create(struct rpc_create_args *args)
> >>>>  		.connect_timeout = args->connect_timeout,
> >>>>  		.reconnect_timeout = args->reconnect_timeout,
> >>>>  	};
> >>>> -	char servername[48];
> >>>> +	char servername[UNIX_PATH_MAX];
> >>>>  	struct rpc_clnt *clnt;
> >>>>  	int i;
> >>>>  
> >>>> -- 
> >>>> 2.43.0
> >>>>
> >>>>
> >>>
> >>
> > 
> 


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ