linux-kernel - Re: [PATCH net-next] af_unix: Refine UNIX domain 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: <20250206042757.3966975-1-buaajxlj@163.com>
Date: Thu,  6 Feb 2025 12:27: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] af_unix: Refine UNIX domain sockets autobind identifier length

From: Kuniyuki Iwashima <kuniyu@...zon.com>,
Date: Thu, 6 Feb 2025 13:01:18 +0900
> > The logs from 'netdev/build_allmodconfig_warn' indicate that the patch has
> > given rise to the following warning:
> > 
> >  - ../net/unix/af_unix.c: In function ‘unix_autobind’:
> >  - ../net/unix/af_unix.c:1227:48: warning: ‘sprintf’ writing a terminating nul past the end of the destination [-Wformat-overflow=]
> >  -  1227 |         sprintf(addr->name->sun_path + 1, "%0*x", AUTOBIND_LEN - 1, ordernum);
> >  -       |                                                ^
> >  - ../net/unix/af_unix.c:1227:9: note: ‘sprintf’ output 6 bytes into a destination of size 5
> >  -  1227 |         sprintf(addr->name->sun_path + 1, "%0*x", AUTOBIND_LEN - 1, ordernum);
> >  -       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > It appears that the 'sprintf' call attempts to write a terminating null
> > byte past the end of the 'sun_path' array, potentially causing an overflow.
> > 
> > To address this issue, I am considering the following approach:
> > 
> > 	char orderstring[6];
> > 
> > 	sprintf(orderstring, "%05x", ordernum);
> > 	memcpy(addr->name->sun_path + 1, orderstring, 5);
> > 
> > This would prevent the buffer overflow by using 'memcpy' to safely copy the
> > formatted string into 'sun_path'.
> 
> Finally new hard-coded values are introduced..
> 
> I'm not sure this is worth saving just 10 bytes, which is not excessive,
> vs extra 5 bytes memcpy(), so I'd rather not touch here.
> 
> > 
> > Before proceeding with a patch submission, I wanted to consult with you to
> > see if you have any suggestions for a better or more elegant solution to
> > this problem.
> 
> An elegant option might be add a variant of snprintf without terminating
> string by \0 ?

Thank you very much for your suggestions. It's an elegant solution that
avoids additional overhead and neatly solves the problem. I appreciate your
insight and will incorporate your idea into the updated patch.

Best regards,
Liang Jie