| 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: <40750d1b-ab1f-4ac4-93ad-97f3bd0aa142@gmail.com>
Date: Thu, 26 Sep 2024 21:58:56 +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/25/24 23:42, NeilBrown wrote:
> 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.
Well, I agree that in this particular case you are correct, but this is not
about you or me being right or wrong, but that a macro would be more descriptive
than "48" or "108", unless you are used to count the beads while chanting mantras
:-)
>> 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).
Ah. I agree, this is why I consider myself a disciple. While in the old
system it was considered asa breach of teacher's authority, other schools
of thinking encouraged questions and debate. ;-)
>> 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.
Yes, my build was somewhat before that date:
-rw-r--r-- 1 root root 18977280 Sep 21 15:44 vmlinuz-6.11.0-gc13-x86-64-tmem-07462-g1868f9d0260e
But I whole-heartedly agree with not taking a literal integer. As we build things to last,
in a "design for life", so I believe that self-documenting source isan advantage to the
counting of places where some integers appear, and what each one meant.
As I said before, this is nothing target SUNRPC implementation, it was among the first compiler
warnings with make W=1 in build. I apologise if that sounded personal.
> Thanks for your interest in improving Linux.
You are most welcome. Thank you for your kind remark.
I am glad that you recognise my bug reports as a positive contribution, despite sometimes (or often)
duplicating work. But, please understand, on my level I do not even have enough disk space for all
the trees, let alone building all of them.
Maybe the best I could promise is to test linux-next and net-next trees.
Probably I could give more meaningful contribution with some expert advice, even when this episode
on SUNRPC was not a waste of time, but an opportunity to learn from the best. :-)
Best regards,
Mirsad Todorovac
> 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