linux-kernel - Re: [PATCH 1/2] staging: vchiq: fix _

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]

Date:   Wed, 23 Sep 2020 07:44:40 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Nicolas Saenz Julienne <nsaenzjulienne@...e.de>,
        devel@...verdev.osuosl.org,
        Marcelo Diop-Gonzalez <marcgonzalez@...gle.com>,
        linux-kernel@...r.kernel.org,
        Nachammai Karuppiah <nachukannan@...il.com>,
        bcm-kernel-feedback-list@...adcom.com,
        linux-rpi-kernel@...ts.infradead.org,
        Jamal Shareef <jamal.k.shareef@...il.com>,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 1/2] staging: vchiq: fix __user annotations

On Tue, Sep 22, 2020 at 10:21:43PM +0200, Arnd Bergmann wrote:
> My earlier patches caused some new sparse warnings, but it turns out
> that a number of those are actual bugs, or at least suspicous code.
> 
> Adding __user annotations to the data structures that are defined in
> uapi headers helps avoid the new warnings, but that causes a different
> set of warnings to show up, as some of these structures are used both
> inside of the kernel and at the user interface but storing pointers to
> different things there.
> 
> Duplicating the vchiq_service_params and vchiq_completion_data structures
> in turn takes care of most of those, and then it turns out that there
> is a 'data' pointer that can be any of a __user address, a dmd_addr_t
> and a kernel pointer in vmalloc space at times.
> 
> I'm trying to annotate these as best I can without changing behavior,
> but there still seems to be a serious bug when user space passes
> a valid vmalloc space address instead of a user pointer. Adding
> comments in the code there, and leaving the warnings in place that
> seem to correspond to actual bugs.
> 
> Signed-off-by: Arnd Bergmann <arnd@...db.de>
> ---
>  .../include/linux/raspberrypi/vchiq.h         | 11 ++-
>  .../interface/vchiq_arm/vchiq_2835_arm.c      |  2 +-
>  .../interface/vchiq_arm/vchiq_arm.c           | 95 ++++++++++++-------
>  .../interface/vchiq_arm/vchiq_core.c          | 19 ++--
>  .../interface/vchiq_arm/vchiq_core.h          | 10 +-
>  .../interface/vchiq_arm/vchiq_ioctl.h         | 29 ++++--
>  6 files changed, 106 insertions(+), 60 deletions(-)

This patch series breaks the build for me:

drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c: In function ‘vc_vchi_audio_init’:
drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c:125:9: error: variable ‘param
’ has initializer but incomplete type
  125 |  struct vchiq_service_params params = {
      |         ^~~~~~~~~~~~~~~~~~~~
drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c:126:4: error: ‘struct vchiq_service_params’ has no member named ‘version’
  126 |   .version  = VC_AUDIOSERV_VER,
      |    ^~~~~~~
In file included from drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c:8:
drivers/staging/vc04_services/bcm2835-audio/vc_vchi_audioserv_defs.h:8:26: warning: excess elements in struct initializer
    8 | #define VC_AUDIOSERV_VER 2
      |                          ^
drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c:126:15: note: in expansion of macro ‘VC_AUDIOSERV_VER’
  126 |   .version  = VC_AUDIOSERV_VER,
      |               ^~~~~~~~~~~~~~~~


and so on...

Care to try a v2?

thanks,

greg k-h