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-next>] [day] [month] [year] [list]
Message-ID: <20230119103446.GO16547@kitsune.suse.cz>
Date:   Thu, 19 Jan 2023 11:34:46 +0100
From:   Michal Suchánek <msuchanek@...e.de>
To:     Christophe Leroy <christophe.leroy@...roup.eu>
Cc:     "Erhard F." <erhard_f@...lbox.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Rob Herring <robh+dt@...nel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE" 
        <devicetree@...r.kernel.org>,
        Frank Rowand <frowand.list@...il.com>,
        Javier Martinez Canillas <javierm@...hat.com>,
        open list <linux-kernel@...r.kernel.org>,
        "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>
Subject: Re: [PATCH v2] of: Fix of platform build on powerpc due to bad of
 disaply code

Hello,

On Thu, Jan 19, 2023 at 10:24:07AM +0000, Christophe Leroy wrote:
> 
> 
> Le 19/01/2023 à 10:53, Michal Suchanek a écrit :
> > The commit 2d681d6a23a1 ("of: Make of framebuffer devices unique")
> > breaks build because of wrong argument to snprintf. That certainly
> > avoids the runtime error but is not the intended outcome.
> > 
> > Also use standard device name format of-display.N for all created
> > devices.
> > 
> > Fixes: 2d681d6a23a1 ("of: Make of framebuffer devices unique")
> > Signed-off-by: Michal Suchanek <msuchanek@...e.de>
> > ---
> > v2: Update the device name format
> > ---
> >   drivers/of/platform.c | 12 ++++++++----
> >   1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > index f2a5d679a324..8c1b1de22036 100644
> > --- a/drivers/of/platform.c
> > +++ b/drivers/of/platform.c
> > @@ -525,7 +525,9 @@ static int __init of_platform_default_populate_init(void)
> >   	if (IS_ENABLED(CONFIG_PPC)) {
> >   		struct device_node *boot_display = NULL;
> >   		struct platform_device *dev;
> > -		int display_number = 1;
> > +		int display_number = 0;
> > +		char buf[14];
> 
> Can you declare that in the for block where it is used instead ?

No, there are two for blocks.

> 
> > +		char *of_display_format = "of-display.%d";
> 
> Should be const ?

Yes, could be.

> 
> >   		int ret;
> >   
> >   		/* Check if we have a MacOS display without a node spec */
> > @@ -556,7 +558,10 @@ static int __init of_platform_default_populate_init(void)
> >   			if (!of_get_property(node, "linux,opened", NULL) ||
> >   			    !of_get_property(node, "linux,boot-display", NULL))
> >   				continue;
> > -			dev = of_platform_device_create(node, "of-display", NULL);
> > +			ret = snprintf(buf, sizeof(buf), of_display_format, display_number++);
> > +			if (ret >= sizeof(buf))
> > +				continue;
> 
> 
> Can you make buf big enough to avoid that ?

It would be a bit fragile that way.

The buffer would have to theoretically accomodate
"of-display.-9223372036854775808", and any change to the format requires
recalculating the length, by hand.

Of course, the memory would run out way before allocating that many
devices so it's kind of pointless to try and accomodate all possible
device numbers.

> 
> And by the way could it be called something else than 'buf' ?
> 
> See exemple here : 
> https://elixir.bootlin.com/linux/v6.1/source/drivers/fsi/fsi-occ.c#L690

Yes, that is quite possible. Nonetheless, just like 'ret' generic
variable names also work.

Thanks

Michal

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ