linux-kernel - Re: [PATCH V2 2/9] staging: vchiq

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>] [day] [month] [year] [list]

Message-ID: <CAGXu5jLHgKg=c69v80qFWeyE8djU3+2ruQu0HvUxT3MMRmd72A@mail.gmail.com>
Date:   Mon, 30 Apr 2018 15:38:22 -0700
From:   Kees Cook <keescook@...gle.com>
To:     Stefan Wahren <stefan.wahren@...e.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Eric Anholt <eric@...olt.net>, Phil Elwell <phil@...pberrypi.org>,
        linux-rpi-kernel@...ts.infradead.org, devel@...verdev.osuosl.org,
        "Tobin C . Harding" <me@...in.cc>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V2 2/9] staging: vchiq_arm: Clear VLA warning

On Sat, Apr 28, 2018 at 8:47 AM, Stefan Wahren <stefan.wahren@...e.com> wrote:
> The kernel would like to have all stack VLA usage removed[1]. The array
> here is fixed (declared with a const variable) but it appears like a VLA
> to the compiler. Also, currently we are putting 768 bytes on the
> stack. This function is only called on the error path so performance is
> not critical, let's just allocate the memory instead of using the
> stack. This saves stack space and removes the VLA build warning.
>
> kmalloc a buffer for dumping state instead of using the stack.
>
> [1]: https://lkml.org/lkml/2018/3/7/621
>
> CC: Kees Cook <keescook@...gle.com>
> Signed-off-by: Tobin C. Harding <me@...in.cc>
> Signed-off-by: Stefan Wahren <stefan.wahren@...e.com>

Reviewed-by: Kees Cook <keescook@...omium.org>

-Kees

> ---
>  .../vc04_services/interface/vchiq_arm/vchiq_arm.c  | 25 ++++++++++++++--------
>  1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> index 8575bd8..d46a5b8 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -3415,13 +3415,18 @@ vchiq_release_service(VCHIQ_SERVICE_HANDLE_T handle)
>         return ret;
>  }
>
> +struct service_data_struct {
> +       int fourcc;
> +       int clientid;
> +       int use_count;
> +};
> +
>  void
>  vchiq_dump_service_use_state(VCHIQ_STATE_T *state)
>  {
>         VCHIQ_ARM_STATE_T *arm_state = vchiq_platform_get_arm_state(state);
> +       struct service_data_struct *service_data;
>         int i, j = 0;
> -       /* Only dump 64 services */
> -       static const int local_max_services = 64;
>         /* If there's more than 64 services, only dump ones with
>          * non-zero counts */
>         int only_nonzero = 0;
> @@ -3432,25 +3437,25 @@ vchiq_dump_service_use_state(VCHIQ_STATE_T *state)
>         int peer_count;
>         int vc_use_count;
>         int active_services;
> -       struct service_data_struct {
> -               int fourcc;
> -               int clientid;
> -               int use_count;
> -       } service_data[local_max_services];
>
>         if (!arm_state)
>                 return;
>
> +       service_data = kmalloc_array(MAX_SERVICES, sizeof(*service_data),
> +                                    GFP_KERNEL);
> +       if (!service_data)
> +               return;
> +
>         read_lock_bh(&arm_state->susp_res_lock);
>         vc_suspend_state = arm_state->vc_suspend_state;
>         vc_resume_state  = arm_state->vc_resume_state;
>         peer_count = arm_state->peer_use_count;
>         vc_use_count = arm_state->videocore_use_count;
>         active_services = state->unused_service;
> -       if (active_services > local_max_services)
> +       if (active_services > MAX_SERVICES)
>                 only_nonzero = 1;
>
> -       for (i = 0; (i < active_services) && (j < local_max_services); i++) {
> +       for (i = 0; (i < active_services) && (j < MAX_SERVICES); i++) {
>                 VCHIQ_SERVICE_T *service_ptr = state->services[i];
>
>                 if (!service_ptr)
> @@ -3494,6 +3499,8 @@ vchiq_dump_service_use_state(VCHIQ_STATE_T *state)
>         vchiq_log_warning(vchiq_susp_log_level,
>                 "--- Overall vchiq instance use count %d", vc_use_count);
>
> +       kfree(service_data);
> +
>         vchiq_dump_platform_use_state(state);
>  }
>
> --
> 2.7.4
>



-- 
Kees Cook
Pixel Security