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  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]
Date:	Sat, 21 May 2016 11:27:53 -0700
From:	Bjorn Andersson <bjorn.andersson@...aro.org>
To:	Andy Yan <andy.yan@...k-chips.com>
Cc:	Rob Herring <robh+dt@...nel.org>, heiko@...ech.de,
	Krzysztof Kozłowski <k.kozlowski@...sung.com>,
	sre@...nel.org, John Stultz <john.stultz@...aro.org>,
	Arnd Bergmann <arnd@...db.de>, Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>, devicetree@...r.kernel.org,
	lkml <linux-kernel@...r.kernel.org>, dbaryshkov@...il.com,
	David Woodhouse <dwmw2@...radead.org>,
	alexandre.belloni@...e-electrons.com, moritz.fischer@...us.com,
	Florian Fainelli <f.fainelli@...il.com>,
	linux-pm@...r.kernel.org
Subject: Re: [PATCH v8 2/4] power: reset: add reboot mode driver

On Sun, Apr 24, 2016 at 11:55 PM, Andy Yan <andy.yan@...k-chips.com> wrote:
[..]
> diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig

[..]

> +config SYSCON_REBOOT_MODE
> +       bool "Generic SYSCON regmap reboot mode driver"
> +       depends on OF

As this isn't really useful without syscon (but still compiles), I
would suggest adding:

depends on SYSCON || COMPILE_TEST

> +       select REBOOT_MODE
> +       help
> +         Say y here will enable reboot mode driver. This will
> +         get reboot mode arguments and store it in SYSCON mapped
> +         register, then the bootloader can read it to take different
> +         action according to the mode.
> +
>  endif
>

[..]

> diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c

[..]

> +
> +#define PREFIX "mode-"
> +
> +struct mode_info {
> +       const char *mode;
> +       unsigned int magic;

If you're using of_property_read_u32() to populate this directly you
should have this as u32.

> +       struct list_head list;
> +};
> +
> +static int get_reboot_mode_magic(struct reboot_mode_driver *reboot,
> +                                const char *cmd)

Magic comes from an unsigned property and is passed as unsigned to the
regmap api. Please make it unsigned int throughout the implementation.

> +{
> +       const char *normal = "normal";
> +       int magic = 0;
> +       struct mode_info *info;
> +
> +       if (!cmd)
> +               cmd = normal;
> +
> +       list_for_each_entry(info, &reboot->head, list) {
> +               if (!strcmp(info->mode, cmd)) {
> +                       magic = info->magic;
> +                       break;
> +               }
> +       }
> +

[..]

> +int reboot_mode_register(struct reboot_mode_driver *reboot)
> +{
> +       struct mode_info *info;
> +       struct property *prop;
> +       struct device_node *np = reboot->dev->of_node;
> +       size_t len = strlen(PREFIX);
> +       int ret;
> +
> +       INIT_LIST_HEAD(&reboot->head);
> +
> +       for_each_property_of_node(np, prop) {
> +               if (len > strlen(prop->name) || strncmp(prop->name, PREFIX, len))

There's no reason to iterate over the string twice here, strncmp will
exit with -1 if you hit a '\0' before strlen(PREFIX). So you can drop
the first check without any difference in functionality.

What passes this checkout though is if the property equals PREFIX, but
this is probably better to handle as an error, rather than skip over.
So do that below

> +                       continue;
> +
> +               info = devm_kzalloc(reboot->dev, sizeof(*info), GFP_KERNEL);
> +               if (!info) {
> +                       ret = -ENOMEM;
> +                       goto error;
> +               }
> +
> +               info->mode = kstrdup_const(prop->name + len, GFP_KERNEL);

You need something like this here:

if (!info->mode) {
    ret = -ENOMEM;
    goto error;
} else if (info->mode[0] == '\0')
    dev_err("mode too short");
    ret = -EINVAL;
    goto error;
}


If you do the kernel a favor and submit a patch to
drivers/base/devres.c adding devm_kstrdup_const() you don't have to do
the goto dance at all.

> +               if (of_property_read_u32(np, prop->name, &info->magic)) {
> +                       dev_err(reboot->dev, "reboot mode %s without magic number\n",
> +                               info->mode);
> +                       devm_kfree(reboot->dev, info);
> +                       continue;
> +               }
> +               list_add_tail(&info->list, &reboot->head);
> +       }
> +
> +       reboot->reboot_notifier.notifier_call = reboot_mode_notify;
> +       ret = register_reboot_notifier(&reboot->reboot_notifier);
> +       if (ret)
> +               dev_err(reboot->dev, "can't register reboot notifier\n");

You're returning an error but haven't freed your info->modes.

> +
> +       return ret;
> +
> +error:
> +       list_for_each_entry(info, &reboot->head, list)
> +               kfree_const(info->mode);
> +
> +       return ret;
> +}

Regards,
Bjorn

Powered by blists - more mailing lists