| 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
| ||
|
Message-id: <172717463033.17050.14835776993804647247@noble.neil.brown.name>
Date: Tue, 24 Sep 2024 20:43:50 +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 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.
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