linux-kernel - Re: [PATCH net-next v2] af_unix: Refine UNIX pathname sockets autobind identifier length

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: <20250206094457.196837-1-buaajxlj@163.com>
Date: Thu,  6 Feb 2025 17:44:57 +0800
From: Liang Jie <buaajxlj@....com>
To: kuniyu@...zon.com
Cc: buaajxlj@....com,
	davem@...emloft.net,
	edumazet@...gle.com,
	horms@...nel.org,
	kuba@...nel.org,
	liangjie@...iang.com,
	linux-kernel@...r.kernel.org,
	mhal@...x.co,
	netdev@...r.kernel.org,
	pabeni@...hat.com
Subject: Re: [PATCH net-next v2] af_unix: Refine UNIX pathname sockets autobind identifier length

From: Kuniyuki Iwashima <kuniyu@...zon.com>
Date: Thu, 6 Feb 2025 17:58:34 +0900
> From: Liang Jie <buaajxlj@....com>
> Date: Thu,  6 Feb 2025 16:19:05 +0800
> > Hi Kuniyuki,
> > 
> > The logs from 'netdev/build_allmodconfig_warn' is as follows:
> >   ../net/unix/af_unix.c: In function ‘unix_autobind’:
> >   ../net/unix/af_unix.c:1222:52: warning: ‘snprintf’ output truncated before the last format character [-Wformat-truncation=]
> >    1222 |         snprintf(addr->name->sun_path + 1, 5, "%05x", ordernum);
> >         |                                                    ^
> >   ../net/unix/af_unix.c:1222:9: note: ‘snprintf’ output 6 bytes into a destination of size 5
> >    1222 |         snprintf(addr->name->sun_path + 1, 5, "%05x", ordernum);
> >         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > snprintf() also append a trailing '\0' at the end of the sun_path.
> 
> I didn't say snprintf() would work rather we need a variant of it that
> does not terminate string with \0.
> 
> 
> > 
> > Now, I think of three options. Which one do you think we should choose?
> > 
> > 1. Allocate an additional byte during the kzalloc phase.
> > 	addr = kzalloc(sizeof(*addr) + offsetof(struct sockaddr_un, sun_path) +
> > 		       UNIX_AUTOBIND_LEN + 1, GFP_KERNEL);
> > 
> > 2. Use temp buffer and memcpy() for handling.
> > 
> > 3. Keep the current code as it is.
> > 
> > Do you have any other suggestions?
> 
> I'd choose 3. as said in v1 thread.  We can't avoid hard-coding and
> adjustment like +1 and -1 here.

The option 3 would result in a waste of ten bytes. Why not choose option 1.

I have a question about the initial use of the hardcoded value 16.
Why was this value chosen and not another?  sizeof(struct sockaddr)?

Its introduction felt abrupt and made understanding the code challenging for me,
which is why I submitted a patch to address this.