| 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: <1430dbab-0540-448b-b503-e53268f60bfc@gmail.com>
Date: Wed, 25 Sep 2024 19:51:19 +0200
From: Mirsad Todorovac <mtodorovac69@...il.com>
To: NeilBrown <neilb@...e.de>
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 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?
Making it logically depend on the system limits might save some headache
in the future, perhaps.
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.
However, you got to this five weeks earlier - but the patch did not
propagate to the main vanilla torvalds tree.
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