linux-kernel - Re: [PATCH] driver core: Fix an uninitialized variable is used by __device

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: <ZsgrXg3JR-Z1z-sr@google.com>
Date: Thu, 22 Aug 2024 23:25:34 -0700
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Zijun Hu <zijun_hu@...oud.com>, "Rafael J. Wysocki" <rafael@...nel.org>,
	linux-kernel@...r.kernel.org, Zijun Hu <quic_zijuhu@...cinc.com>,
	stable@...r.kernel.org
Subject: Re: [PATCH] driver core: Fix an uninitialized variable is used by
 __device_attach()

On Fri, Aug 23, 2024 at 02:11:45PM +0800, Greg Kroah-Hartman wrote:
> On Thu, Aug 22, 2024 at 06:25:15PM -0700, Dmitry Torokhov wrote:
> > On Fri, Aug 23, 2024 at 09:14:12AM +0800, Greg Kroah-Hartman wrote:
> > > On Fri, Aug 23, 2024 at 08:46:12AM +0800, Zijun Hu wrote:
> > > > On 2024/8/23 08:02, Dmitry Torokhov wrote:
> > > > > Hi,
> > > > > 
> > > > > On Fri, Aug 23, 2024 at 07:46:09AM +0800, Zijun Hu wrote:
> > > > >> From: Zijun Hu <quic_zijuhu@...cinc.com>
> > > > >>
> > > > >> An uninitialized variable @data.have_async may be used as analyzed
> > > > >> by the following inline comments:
> > > > >>
> > > > >> static int __device_attach(struct device *dev, bool allow_async)
> > > > >> {
> > > > >> 	// if @allow_async is true.
> > > > >>
> > > > >> 	...
> > > > >> 	struct device_attach_data data = {
> > > > >> 		.dev = dev,
> > > > >> 		.check_async = allow_async,
> > > > >> 		.want_async = false,
> > > > >> 	};
> > > > >> 	// @data.have_async is not initialized.
> > > > > 
> > > > > No, in the presence of a structure initializer fields not explicitly
> > > > > initialized will be set to 0 by the compiler.
> > > > > 
> > > > really?
> > > > do all C compilers have such behavior ?
> > > 
> > > Oh wait, if this were static, then yes, it would all be set to 0, sorry,
> > > I misread this.
> > > 
> > > This is on the stack so it needs to be zeroed out explicitly.  We should
> > > set the whole thing to 0 and then set only the fields we want to
> > > override to ensure it's all correct.
> > 
> > No we do not. ISO/IEC 9899:201x 6.7.9 Initialization:
> > 
> > "21 If there are fewer initializers in a brace-enclosed list than there
> > are elements or members of an aggregate, or fewer characters in a string
> > literal used to initialize an array of known size than there are
> > elements in the array, the remainder of the aggregate shall be
> > initialized implicitly the same as objects that have static storage
> > duration."
> > 
> > That is why you can 0-initialize a structure by doing:
> > 
> > 	struct s s1 = { 0 };
> > 
> > or even
> > 
> > 	struct s s1 = { };
> 
> {sigh}  I always get this wrong, also there's the question "are holes
> in the structure also set to 0" which as you can see from the above
> spec, should also be true.  But numerous places in the kernel explicitly
> use memset() to "make sure" of that.

I think it has more to do with our preference for having declarations
before code, so if there is complex or conditional initialization then
it is more natural to declare uninitialized variable, and then later
explicitly memset() it and assign required values to members.

Thanks.

-- 
Dmitry