diff mbox series

[1/2] staging: vchiq: fix __user annotations

Message ID 20200922202208.1861595-1-arnd@arndb.de
State New, archived
Headers show
Series [1/2] staging: vchiq: fix __user annotations | expand

Commit Message

Arnd Bergmann Sept. 22, 2020, 8:21 p.m. UTC
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@arndb.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(-)

Comments

kernel test robot Sept. 23, 2020, 2:02 a.m. UTC | #1
Hi Arnd,

I love your patch! Yet something to improve:

[auto build test ERROR on staging/staging-testing]

url:    https://github.com/0day-ci/linux/commits/Arnd-Bergmann/staging-vchiq-fix-__user-annotations/20200923-042315
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git 69fea2b4e59c52844cf5196c9c81157792d194fb
config: nds32-allyesconfig (attached as .config)
compiler: nds32le-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=nds32 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

   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 'params' 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,
         |               ^~~~~~~~~~~~~~~~
   drivers/staging/vc04_services/bcm2835-audio/vc_vchi_audioserv_defs.h:8:26: note: (near initialization for 'params')
       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,
         |               ^~~~~~~~~~~~~~~~
>> drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c:127:4: error: 'struct vchiq_service_params' has no member named 'version_min'
     127 |   .version_min  = VC_AUDIOSERV_MIN_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:7:30: warning: excess elements in struct initializer
       7 | #define VC_AUDIOSERV_MIN_VER 1
         |                              ^
   drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c:127:19: note: in expansion of macro 'VC_AUDIOSERV_MIN_VER'
     127 |   .version_min  = VC_AUDIOSERV_MIN_VER,
         |                   ^~~~~~~~~~~~~~~~~~~~
   drivers/staging/vc04_services/bcm2835-audio/vc_vchi_audioserv_defs.h:7:30: note: (near initialization for 'params')
       7 | #define VC_AUDIOSERV_MIN_VER 1
         |                              ^
   drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c:127:19: note: in expansion of macro 'VC_AUDIOSERV_MIN_VER'
     127 |   .version_min  = VC_AUDIOSERV_MIN_VER,
         |                   ^~~~~~~~~~~~~~~~~~~~
>> drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c:128:4: error: 'struct vchiq_service_params' has no member named 'fourcc'
     128 |   .fourcc   = VCHIQ_MAKE_FOURCC('A', 'U', 'D', 'S'),
         |    ^~~~~~
   In file included from drivers/staging/vc04_services/bcm2835-audio/bcm2835.h:9,
                    from drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c:7:
>> drivers/staging/vc04_services/bcm2835-audio/../include/linux/raspberrypi/vchiq.h:8:4: warning: excess elements in struct initializer
       8 |    (((x0) << 24) | ((x1) << 16) | ((x2) << 8) | (x3))
         |    ^
   drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c:128:15: note: in expansion of macro 'VCHIQ_MAKE_FOURCC'
     128 |   .fourcc   = VCHIQ_MAKE_FOURCC('A', 'U', 'D', 'S'),
         |               ^~~~~~~~~~~~~~~~~
   drivers/staging/vc04_services/bcm2835-audio/../include/linux/raspberrypi/vchiq.h:8:4: note: (near initialization for 'params')
       8 |    (((x0) << 24) | ((x1) << 16) | ((x2) << 8) | (x3))
         |    ^
   drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c:128:15: note: in expansion of macro 'VCHIQ_MAKE_FOURCC'
     128 |   .fourcc   = VCHIQ_MAKE_FOURCC('A', 'U', 'D', 'S'),
         |               ^~~~~~~~~~~~~~~~~
>> drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c:129:4: error: 'struct vchiq_service_params' has no member named 'callback'
     129 |   .callback  = audio_vchi_callback,
         |    ^~~~~~~~
>> drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c:129:16: warning: excess elements in struct initializer
     129 |   .callback  = audio_vchi_callback,
         |                ^~~~~~~~~~~~~~~~~~~
   drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c:129:16: note: (near initialization for 'params')
>> drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c:130:4: error: 'struct vchiq_service_params' has no member named 'userdata'
     130 |   .userdata  = instance,
         |    ^~~~~~~~
   drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c:130:16: warning: excess elements in struct initializer
     130 |   .userdata  = instance,
         |                ^~~~~~~~
   drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c:130:16: note: (near initialization for 'params')
>> drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c:125:30: error: storage size of 'params' isn't known
     125 |  struct vchiq_service_params params = {
         |                              ^~~~~~
   drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c:125:30: warning: unused variable 'params' [-Wunused-variable]
--
   drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c: In function 'vchiq_mmal_init':
>> drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c:1861:9: error: variable 'params' has initializer but incomplete type
    1861 |  struct vchiq_service_params params = {
         |         ^~~~~~~~~~~~~~~~~~~~
>> drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c:1862:4: error: 'struct vchiq_service_params' has no member named 'version'
    1862 |   .version  = VC_MMAL_VER,
         |    ^~~~~~~
   In file included from drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c:31:
>> drivers/staging/vc04_services/vchiq-mmal/mmal-msg.h:29:21: warning: excess elements in struct initializer
      29 | #define VC_MMAL_VER 15
         |                     ^~
   drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c:1862:15: note: in expansion of macro 'VC_MMAL_VER'
    1862 |   .version  = VC_MMAL_VER,
         |               ^~~~~~~~~~~
   drivers/staging/vc04_services/vchiq-mmal/mmal-msg.h:29:21: note: (near initialization for 'params')
      29 | #define VC_MMAL_VER 15
         |                     ^~
   drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c:1862:15: note: in expansion of macro 'VC_MMAL_VER'
    1862 |   .version  = VC_MMAL_VER,
         |               ^~~~~~~~~~~
>> drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c:1863:4: error: 'struct vchiq_service_params' has no member named 'version_min'
    1863 |   .version_min  = VC_MMAL_MIN_VER,
         |    ^~~~~~~~~~~
   In file included from drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c:31:
   drivers/staging/vc04_services/vchiq-mmal/mmal-msg.h:30:25: warning: excess elements in struct initializer
      30 | #define VC_MMAL_MIN_VER 10
         |                         ^~
   drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c:1863:19: note: in expansion of macro 'VC_MMAL_MIN_VER'
    1863 |   .version_min  = VC_MMAL_MIN_VER,
         |                   ^~~~~~~~~~~~~~~
   drivers/staging/vc04_services/vchiq-mmal/mmal-msg.h:30:25: note: (near initialization for 'params')
      30 | #define VC_MMAL_MIN_VER 10
         |                         ^~
   drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c:1863:19: note: in expansion of macro 'VC_MMAL_MIN_VER'
    1863 |   .version_min  = VC_MMAL_MIN_VER,
         |                   ^~~~~~~~~~~~~~~
>> drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c:1864:4: error: 'struct vchiq_service_params' has no member named 'fourcc'
    1864 |   .fourcc   = VCHIQ_MAKE_FOURCC('m', 'm', 'a', 'l'),
         |    ^~~~~~
   In file included from drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c:26:
>> drivers/staging/vc04_services/vchiq-mmal/../include/linux/raspberrypi/vchiq.h:8:4: warning: excess elements in struct initializer
       8 |    (((x0) << 24) | ((x1) << 16) | ((x2) << 8) | (x3))
         |    ^
   drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c:1864:15: note: in expansion of macro 'VCHIQ_MAKE_FOURCC'
    1864 |   .fourcc   = VCHIQ_MAKE_FOURCC('m', 'm', 'a', 'l'),
         |               ^~~~~~~~~~~~~~~~~
   drivers/staging/vc04_services/vchiq-mmal/../include/linux/raspberrypi/vchiq.h:8:4: note: (near initialization for 'params')
       8 |    (((x0) << 24) | ((x1) << 16) | ((x2) << 8) | (x3))
         |    ^
   drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c:1864:15: note: in expansion of macro 'VCHIQ_MAKE_FOURCC'
    1864 |   .fourcc   = VCHIQ_MAKE_FOURCC('m', 'm', 'a', 'l'),
         |               ^~~~~~~~~~~~~~~~~
>> drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c:1865:4: error: 'struct vchiq_service_params' has no member named 'callback'
    1865 |   .callback  = service_callback,
         |    ^~~~~~~~
>> drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c:1865:16: warning: excess elements in struct initializer
    1865 |   .callback  = service_callback,
         |                ^~~~~~~~~~~~~~~~
   drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c:1865:16: note: (near initialization for 'params')
>> drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c:1866:4: error: 'struct vchiq_service_params' has no member named 'userdata'
    1866 |   .userdata  = NULL,
         |    ^~~~~~~~
   In file included from include/uapi/linux/posix_types.h:5,
                    from include/uapi/linux/types.h:14,
                    from include/linux/types.h:6,
                    from include/linux/limits.h:6,
                    from include/linux/kernel.h:7,
                    from drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c:19:
   include/linux/stddef.h:8:14: warning: excess elements in struct initializer
       8 | #define NULL ((void *)0)
         |              ^
   drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c:1866:16: note: in expansion of macro 'NULL'
    1866 |   .userdata  = NULL,
         |                ^~~~
   include/linux/stddef.h:8:14: note: (near initialization for 'params')
       8 | #define NULL ((void *)0)
         |              ^
   drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c:1866:16: note: in expansion of macro 'NULL'
    1866 |   .userdata  = NULL,
         |                ^~~~
>> drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c:1861:30: error: storage size of 'params' isn't known
    1861 |  struct vchiq_service_params params = {
         |                              ^~~~~~
   drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c:1861:30: warning: unused variable 'params' [-Wunused-variable]

# https://github.com/0day-ci/linux/commit/7e3c995c9a6394a86c53108753c8c2d9034410e5
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Arnd-Bergmann/staging-vchiq-fix-__user-annotations/20200923-042315
git checkout 7e3c995c9a6394a86c53108753c8c2d9034410e5
vim +126 drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c

23b028c871e113 drivers/staging/bcm2835-audio/bcm2835-vchiq.c               Michael Zoran          2017-01-25  120  
d7ca3a71545bae drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c Takashi Iwai           2018-09-04  121  static int
a7983fd9462560 drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c Nicolas Saenz Julienne 2020-06-29  122  vc_vchi_audio_init(struct vchiq_instance *vchiq_instance,
d7ca3a71545bae drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c Takashi Iwai           2018-09-04  123  		   struct bcm2835_audio_instance *instance)
23b028c871e113 drivers/staging/bcm2835-audio/bcm2835-vchiq.c               Michael Zoran          2017-01-25  124  {
65c7536672b9ee drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c Nicolas Saenz Julienne 2020-06-29 @125  	struct vchiq_service_params params = {
65c7536672b9ee drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c Nicolas Saenz Julienne 2020-06-29 @126  		.version		= VC_AUDIOSERV_VER,
65c7536672b9ee drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c Nicolas Saenz Julienne 2020-06-29 @127  		.version_min		= VC_AUDIOSERV_MIN_VER,
b06eba5c525aed drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c Nicolas Saenz Julienne 2020-06-29 @128  		.fourcc			= VCHIQ_MAKE_FOURCC('A', 'U', 'D', 'S'),
f5a3db42e8a6a9 drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c Takashi Iwai           2018-09-04 @129  		.callback		= audio_vchi_callback,
65c7536672b9ee drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c Nicolas Saenz Julienne 2020-06-29 @130  		.userdata		= instance,
f5a3db42e8a6a9 drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c Takashi Iwai           2018-09-04  131  	};
23b028c871e113 drivers/staging/bcm2835-audio/bcm2835-vchiq.c               Michael Zoran          2017-01-25  132  	int status;
23b028c871e113 drivers/staging/bcm2835-audio/bcm2835-vchiq.c               Michael Zoran          2017-01-25  133  
23b028c871e113 drivers/staging/bcm2835-audio/bcm2835-vchiq.c               Michael Zoran          2017-01-25  134  	/* Open the VCHI service connections */
9d52311134e969 drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c Nicolas Saenz Julienne 2020-06-29  135  	status = vchiq_open_service(vchiq_instance, &params,
3a8895a9219f6b drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c Nicolas Saenz Julienne 2020-06-29  136  				    &instance->service_handle);
c8280337226fc4 drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c Aishwarya Pant         2017-03-03  137  
23b028c871e113 drivers/staging/bcm2835-audio/bcm2835-vchiq.c               Michael Zoran          2017-01-25  138  	if (status) {
435ba133f96eef drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c Takashi Iwai           2018-09-04  139  		dev_err(instance->dev,
435ba133f96eef drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c Takashi Iwai           2018-09-04  140  			"failed to open VCHI service connection (status=%d)\n",
435ba133f96eef drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c Takashi Iwai           2018-09-04  141  			status);
d7ca3a71545bae drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c Takashi Iwai           2018-09-04  142  		return -EPERM;
23b028c871e113 drivers/staging/bcm2835-audio/bcm2835-vchiq.c               Michael Zoran          2017-01-25  143  	}
f5a3db42e8a6a9 drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c Takashi Iwai           2018-09-04  144  
23b028c871e113 drivers/staging/bcm2835-audio/bcm2835-vchiq.c               Michael Zoran          2017-01-25  145  	/* Finished with the service for now */
9d52311134e969 drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c Nicolas Saenz Julienne 2020-06-29  146  	vchiq_release_service(instance->service_handle);
23b028c871e113 drivers/staging/bcm2835-audio/bcm2835-vchiq.c               Michael Zoran          2017-01-25  147  
d7ca3a71545bae drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c Takashi Iwai           2018-09-04  148  	return 0;
23b028c871e113 drivers/staging/bcm2835-audio/bcm2835-vchiq.c               Michael Zoran          2017-01-25  149  }
23b028c871e113 drivers/staging/bcm2835-audio/bcm2835-vchiq.c               Michael Zoran          2017-01-25  150  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Greg Kroah-Hartman Sept. 23, 2020, 5:44 a.m. UTC | #2
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@arndb.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
Arnd Bergmann Sept. 25, 2020, 11:42 a.m. UTC | #3
On Wed, Sep 23, 2020 at 7:44 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> and so on...
>
> Care to try a v2?

I had a look now and found the problem, as there are two drivers that I did not
have enabled but that need a trivial change to match my other modifications.

I'll send the new version in a bit, changes below for reference.

    Arnd

index 292fcee9d6f2..d567a2e3f70c 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -122,7 +122,7 @@ static int
 vc_vchi_audio_init(struct vchiq_instance *vchiq_instance,
                   struct bcm2835_audio_instance *instance)
 {
-       struct vchiq_service_params params = {
+       struct vchiq_service_params_kernel params = {
                .version                = VC_AUDIOSERV_VER,
                .version_min            = VC_AUDIOSERV_MIN_VER,
                .fourcc                 = VCHIQ_MAKE_FOURCC('A', 'U', 'D', 'S'),
diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
index e798d494f00f..3a4202551cfc 100644
--- a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
+++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
@@ -1858,7 +1858,7 @@ int vchiq_mmal_init(struct vchiq_mmal_instance
**out_instance)
        int status;
        struct vchiq_mmal_instance *instance;
        static struct vchiq_instance *vchiq_instance;
-       struct vchiq_service_params params = {
+       struct vchiq_service_params_kernel params = {
                .version                = VC_MMAL_VER,
                .version_min            = VC_MMAL_MIN_VER,
                .fourcc                 = VCHIQ_MAKE_FOURCC('m', 'm', 'a', 'l'),
diff mbox series

Patch

diff --git a/drivers/staging/vc04_services/include/linux/raspberrypi/vchiq.h b/drivers/staging/vc04_services/include/linux/raspberrypi/vchiq.h
index 18d63df368c4..fefc664eefcf 100644
--- a/drivers/staging/vc04_services/include/linux/raspberrypi/vchiq.h
+++ b/drivers/staging/vc04_services/include/linux/raspberrypi/vchiq.h
@@ -62,7 +62,14 @@  struct vchiq_service_base {
 	void *userdata;
 };
 
-struct vchiq_service_params {
+struct vchiq_completion_data_kernel {
+	enum vchiq_reason reason;
+	struct vchiq_header *header;
+	void *service_userdata;
+	void *bulk_userdata;
+};
+
+struct vchiq_service_params_kernel {
 	int fourcc;
 	enum vchiq_status (*callback)(enum vchiq_reason reason,
 				      struct vchiq_header *header,
@@ -79,7 +86,7 @@  extern enum vchiq_status vchiq_initialise(struct vchiq_instance **pinstance);
 extern enum vchiq_status vchiq_shutdown(struct vchiq_instance *instance);
 extern enum vchiq_status vchiq_connect(struct vchiq_instance *instance);
 extern enum vchiq_status vchiq_open_service(struct vchiq_instance *instance,
-	const struct vchiq_service_params *params,
+	const struct vchiq_service_params_kernel *params,
 	unsigned int *pservice);
 extern enum vchiq_status vchiq_close_service(unsigned int service);
 extern enum vchiq_status vchiq_use_service(unsigned int service);
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
index 5ed36d557014..7dc7441db592 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
@@ -229,7 +229,7 @@  vchiq_prepare_bulk_data(struct vchiq_bulk *bulk, void *offset, int size,
 	if (!pagelistinfo)
 		return VCHIQ_ERROR;
 
-	bulk->data = (void *)(unsigned long)pagelistinfo->dma_addr;
+	bulk->data = pagelistinfo->dma_addr;
 
 	/*
 	 * Store the pagelistinfo address in remote_data,
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 bb0cc9cb96e9..4fb19e68eadf 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -53,7 +53,7 @@  int vchiq_susp_log_level = VCHIQ_LOG_ERROR;
 
 struct user_service {
 	struct vchiq_service *service;
-	void *userdata;
+	void __user *userdata;
 	struct vchiq_instance *instance;
 	char is_vchi;
 	char dequeue_pending;
@@ -75,7 +75,7 @@  struct bulk_waiter_node {
 
 struct vchiq_instance {
 	struct vchiq_state *state;
-	struct vchiq_completion_data completions[MAX_COMPLETIONS];
+	struct vchiq_completion_data_kernel completions[MAX_COMPLETIONS];
 	int completion_insert;
 	int completion_remove;
 	struct completion insert_event;
@@ -273,7 +273,7 @@  EXPORT_SYMBOL(vchiq_connect);
 
 static enum vchiq_status vchiq_add_service(
 	struct vchiq_instance             *instance,
-	const struct vchiq_service_params *params,
+	const struct vchiq_service_params_kernel *params,
 	unsigned int       *phandle)
 {
 	enum vchiq_status status;
@@ -311,7 +311,7 @@  static enum vchiq_status vchiq_add_service(
 
 enum vchiq_status vchiq_open_service(
 	struct vchiq_instance             *instance,
-	const struct vchiq_service_params *params,
+	const struct vchiq_service_params_kernel *params,
 	unsigned int       *phandle)
 {
 	enum vchiq_status   status = VCHIQ_ERROR;
@@ -359,7 +359,8 @@  vchiq_bulk_transmit(unsigned int handle, const void *data,
 		switch (mode) {
 		case VCHIQ_BULK_MODE_NOCALLBACK:
 		case VCHIQ_BULK_MODE_CALLBACK:
-			status = vchiq_bulk_transfer(handle, (void *)data, size,
+			status = vchiq_bulk_transfer(handle,
+						     (void *)data, size,
 						     userdata, mode,
 						     VCHIQ_BULK_TRANSMIT);
 			break;
@@ -453,7 +454,8 @@  vchiq_blocking_bulk_transfer(unsigned int handle, void *data,
 
 		if (bulk) {
 			/* This thread has an outstanding bulk transfer. */
-			if ((bulk->data != data) ||
+			/* FIXME: why compare a dma address to a pointer? */
+			if ((bulk->data != (dma_addr_t)(uintptr_t)data) ||
 				(bulk->size != size)) {
 				/* This is not a retry of the previous one.
 				 * Cancel the signal when the transfer
@@ -513,7 +515,7 @@  add_completion(struct vchiq_instance *instance, enum vchiq_reason reason,
 	       struct vchiq_header *header, struct user_service *user_service,
 	       void *bulk_userdata)
 {
-	struct vchiq_completion_data *completion;
+	struct vchiq_completion_data_kernel *completion;
 	int insert;
 
 	DEBUG_INITIALISE(g_state.local)
@@ -802,7 +804,7 @@  static int vchiq_ioc_create_service(struct vchiq_instance *instance,
 	struct user_service *user_service = NULL;
 	struct vchiq_service *service;
 	enum vchiq_status status = VCHIQ_SUCCESS;
-	void *userdata;
+	struct vchiq_service_params_kernel params;
 	int srvstate;
 
 	user_service = kmalloc(sizeof(*user_service), GFP_KERNEL);
@@ -820,20 +822,23 @@  static int vchiq_ioc_create_service(struct vchiq_instance *instance,
 			 VCHIQ_SRVSTATE_LISTENING : VCHIQ_SRVSTATE_HIDDEN;
 	}
 
-	userdata = args->params.userdata;
-	args->params.callback = service_callback;
-	args->params.userdata = user_service;
-	service = vchiq_add_service_internal(instance->state, &args->params,
+	params = (struct vchiq_service_params_kernel) {
+		.fourcc   = args->params.fourcc,
+		.callback = service_callback,
+		.userdata = user_service,
+		.version  = args->params.version,
+		.version_min = args->params.version_min,
+	};
+	service = vchiq_add_service_internal(instance->state, &params,
 					     srvstate, instance,
 					     user_service_free);
-
 	if (!service) {
 		kfree(user_service);
 		return -EEXIST;
 	}
 
 	user_service->service = service;
-	user_service->userdata = userdata;
+	user_service->userdata = args->params.userdata;
 	user_service->instance = instance;
 	user_service->is_vchi = (args->is_vchi != 0);
 	user_service->dequeue_pending = 0;
@@ -945,6 +950,7 @@  static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance *instance,
 {
 	struct vchiq_service *service;
 	struct bulk_waiter_node *waiter = NULL;
+	void *userdata;
 	int status = 0;
 	int ret;
 
@@ -960,7 +966,7 @@  static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance *instance,
 			goto out;
 		}
 
-		args->userdata = &waiter->bulk_waiter;
+		userdata = &waiter->bulk_waiter;
 	} else if (args->mode == VCHIQ_BULK_MODE_WAITING) {
 		mutex_lock(&instance->bulk_waiter_list_mutex);
 		list_for_each_entry(waiter, &instance->bulk_waiter_list,
@@ -981,11 +987,18 @@  static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance *instance,
 		vchiq_log_info(vchiq_arm_log_level,
 			"found bulk_waiter %pK for pid %d", waiter,
 			current->pid);
-		args->userdata = &waiter->bulk_waiter;
+		userdata = &waiter->bulk_waiter;
 	}
 
+	/*
+	 * FIXME address space mismatch:
+	 * args->data may be interpreted as a kernel pointer
+	 * in create_pagelist() called from vchiq_bulk_transfer(),
+	 * accessing kernel data instead of user space, based on the
+	 * address.
+	 */
 	status = vchiq_bulk_transfer(args->handle, args->data, args->size,
-				     args->userdata, args->mode, dir);
+				     userdata, args->mode, dir);
 
 	if (!waiter) {
 		ret = 0;
@@ -1027,19 +1040,22 @@  static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance *instance,
 	return 0;
 }
 
+/* read a user pointer value from an array pointers in user space */
 static inline int vchiq_get_user_ptr(void __user **buf, void __user *ubuf, int index)
 {
-	compat_uptr_t ptr32;
 	int ret;
 
 	if (in_compat_syscall()) {
+		compat_uptr_t ptr32;
 		compat_uptr_t __user *uptr = ubuf;
-		ret = get_user(ptr32, &uptr[index]);
+		ret = get_user(ptr32, uptr + index);
 		*buf = compat_ptr(ptr32);
 	} else {
-		void __user *__user *uptr = ubuf;
-		ret = get_user(buf, &uptr[index]);
+		uintptr_t ptr, __user *uptr = ubuf;
+		ret = get_user(ptr, uptr + index);
+		*buf = (void __user *)ptr;
 	}
+
 	return ret;
 }
 
@@ -1058,10 +1074,10 @@  static int vchiq_put_completion(struct vchiq_completion_data __user *buf,
 
 	if (in_compat_syscall()) {
 		struct vchiq_completion_data32 tmp = {
-			.reason		  = buf->reason,
-			.header		  = ptr_to_compat(buf->header),
-			.service_userdata = ptr_to_compat(buf->service_userdata),
-			.bulk_userdata	  = ptr_to_compat(buf->bulk_userdata),
+			.reason		  = completion->reason,
+			.header		  = ptr_to_compat(completion->header),
+			.service_userdata = ptr_to_compat(completion->service_userdata),
+			.bulk_userdata	  = ptr_to_compat(completion->bulk_userdata),
 		};
 		if (copy_to_user(&buf32[index], &tmp, sizeof(tmp)))
 			return -EFAULT;
@@ -1115,7 +1131,8 @@  static int vchiq_ioc_await_completion(struct vchiq_instance *instance,
 	remove = instance->completion_remove;
 
 	for (ret = 0; ret < args->count; ret++) {
-		struct vchiq_completion_data *completion;
+		struct vchiq_completion_data_kernel *completion;
+		struct vchiq_completion_data user_completion;
 		struct vchiq_service *service;
 		struct user_service *user_service;
 		struct vchiq_header *header;
@@ -1134,7 +1151,12 @@  static int vchiq_ioc_await_completion(struct vchiq_instance *instance,
 
 		service = completion->service_userdata;
 		user_service = service->base.userdata;
-		completion->service_userdata = user_service->userdata;
+
+		memset(&user_completion, 0, sizeof(user_completion));
+		user_completion = (struct vchiq_completion_data) {
+			.reason = completion->reason,
+			.service_userdata = user_service->userdata,
+		};
 
 		header = completion->header;
 		if (header) {
@@ -1158,7 +1180,7 @@  static int vchiq_ioc_await_completion(struct vchiq_instance *instance,
 				break;
 			/* Get the pointer from user space */
 			msgbufcount--;
-			if (vchiq_get_user_ptr(&msgbuf, &args->msgbufs,
+			if (vchiq_get_user_ptr(&msgbuf, args->msgbufs,
 						msgbufcount)) {
 				if (ret == 0)
 					ret = -EFAULT;
@@ -1178,15 +1200,20 @@  static int vchiq_ioc_await_completion(struct vchiq_instance *instance,
 
 			/* The completion must point to the
 			** msgbuf. */
-			completion->header =
-				(struct vchiq_header __force *)msgbuf;
+			user_completion.header = msgbuf;
 		}
 
 		if ((completion->reason == VCHIQ_SERVICE_CLOSED) &&
 		    !instance->use_close_delivered)
 			unlock_service(service);
 
-		if (vchiq_put_completion(args->buf, completion, ret)) {
+		/*
+		 * FIXME: address space mismatch, does bulk_userdata
+		 * actually point to user or kernel memory?
+		 */
+		user_completion.bulk_userdata = completion->bulk_userdata;
+
+		if (vchiq_put_completion(args->buf, &user_completion, ret)) {
 			if (ret == 0)
 				ret = -EFAULT;
 			break;
@@ -1713,7 +1740,7 @@  struct vchiq_await_completion32 {
 static long
 vchiq_compat_ioctl_await_completion(struct file *file,
 				    unsigned int cmd,
-				    struct vchiq_await_completion32 *argp)
+				    struct vchiq_await_completion32 __user *argp)
 {
 	struct vchiq_await_completion args;
 	struct vchiq_await_completion32 args32;
@@ -1926,7 +1953,7 @@  static int vchiq_release(struct inode *inode, struct file *file)
 	/* Release any closed services */
 	while (instance->completion_remove !=
 		instance->completion_insert) {
-		struct vchiq_completion_data *completion;
+		struct vchiq_completion_data_kernel *completion;
 		struct vchiq_service *service;
 
 		completion = &instance->completions[
@@ -2191,7 +2218,7 @@  vchiq_keepalive_thread_func(void *v)
 	struct vchiq_instance *instance;
 	unsigned int ka_handle;
 
-	struct vchiq_service_params params = {
+	struct vchiq_service_params_kernel params = {
 		.fourcc      = VCHIQ_MAKE_FOURCC('K', 'E', 'E', 'P'),
 		.callback    = vchiq_keepalive_vchiq_callback,
 		.version     = KEEPALIVE_VER,
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index 5a361e8e7c6c..12110692e68d 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -1392,7 +1392,7 @@  abort_outstanding_bulks(struct vchiq_service *service,
 				bulk->remote_size);
 		} else {
 			/* fabricate a matching dummy bulk */
-			bulk->data = NULL;
+			bulk->data = 0;
 			bulk->size = 0;
 			bulk->actual = VCHIQ_BULK_ACTUAL_ABORTED;
 			bulk->dir = is_tx ? VCHIQ_BULK_TRANSMIT :
@@ -1764,10 +1764,10 @@  parse_rx_slots(struct vchiq_state *state)
 				queue->remote_insert++;
 
 				vchiq_log_info(vchiq_core_log_level,
-					"%d: prs %s@%pK (%d->%d) %x@%pK",
+					"%d: prs %s@%pK (%d->%d) %x@%pad",
 					state->id, msg_type_str(type),
 					header, remoteport, localport,
-					bulk->actual, bulk->data);
+					bulk->actual, &bulk->data);
 
 				vchiq_log_trace(vchiq_core_log_level,
 					"%d: prs:%d %cx li=%x ri=%x p=%x",
@@ -2316,7 +2316,7 @@  struct vchiq_header *vchiq_msg_hold(unsigned int handle)
 }
 EXPORT_SYMBOL(vchiq_msg_hold);
 
-static int vchiq_validate_params(const struct vchiq_service_params *params)
+static int vchiq_validate_params(const struct vchiq_service_params_kernel *params)
 {
 	if (!params->callback || !params->fourcc) {
 		vchiq_loud_error("Can't add service, invalid params\n");
@@ -2329,7 +2329,7 @@  static int vchiq_validate_params(const struct vchiq_service_params *params)
 /* Called from application thread when a client or server service is created. */
 struct vchiq_service *
 vchiq_add_service_internal(struct vchiq_state *state,
-			   const struct vchiq_service_params *params,
+			   const struct vchiq_service_params_kernel *params,
 			   int srvstate, struct vchiq_instance *instance,
 			   vchiq_userdata_term userdata_term)
 {
@@ -3015,7 +3015,8 @@  vchiq_remove_service(unsigned int handle)
  * structure.
  */
 enum vchiq_status vchiq_bulk_transfer(unsigned int handle,
-				   void *offset, int size, void *userdata,
+				   void *offset, int size,
+				   void *userdata,
 				   enum vchiq_bulk_mode mode,
 				   enum vchiq_bulk_dir dir)
 {
@@ -3093,9 +3094,9 @@  enum vchiq_status vchiq_bulk_transfer(unsigned int handle,
 	wmb();
 
 	vchiq_log_info(vchiq_core_log_level,
-		"%d: bt (%d->%d) %cx %x@%pK %pK",
+		"%d: bt (%d->%d) %cx %x@%pad %pK",
 		state->id, service->localport, service->remoteport, dir_char,
-		size, bulk->data, userdata);
+		size, &bulk->data, userdata);
 
 	/* The slot mutex must be held when the service is being closed, so
 	   claim it here to ensure that isn't happening */
@@ -3107,7 +3108,7 @@  enum vchiq_status vchiq_bulk_transfer(unsigned int handle,
 	if (service->srvstate != VCHIQ_SRVSTATE_OPEN)
 		goto unlock_both_error_exit;
 
-	payload[0] = (int)(long)bulk->data;
+	payload[0] = lower_32_bits(bulk->data);
 	payload[1] = bulk->size;
 	status = queue_message(state,
 			       NULL,
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
index e67692879249..5ec717969676 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -231,7 +231,7 @@  struct vchiq_bulk {
 	short mode;
 	short dir;
 	void *userdata;
-	void *data;
+	dma_addr_t data;
 	int size;
 	void *remote_data;
 	int remote_size;
@@ -534,9 +534,9 @@  vchiq_init_state(struct vchiq_state *state, struct vchiq_slot_zero *slot_zero);
 extern enum vchiq_status
 vchiq_connect_internal(struct vchiq_state *state, struct vchiq_instance *instance);
 
-extern struct vchiq_service *
+struct vchiq_service *
 vchiq_add_service_internal(struct vchiq_state *state,
-			   const struct vchiq_service_params *params,
+			   const struct vchiq_service_params_kernel *params,
 			   int srvstate, struct vchiq_instance *instance,
 			   vchiq_userdata_term userdata_term);
 
@@ -632,8 +632,8 @@  vchiq_queue_message(unsigned int handle,
 ** implementations must be provided. */
 
 extern enum vchiq_status
-vchiq_prepare_bulk_data(struct vchiq_bulk *bulk, void *offset, int size,
-			int dir);
+vchiq_prepare_bulk_data(struct vchiq_bulk *bulk, void *offset,
+			int size, int dir);
 
 extern void
 vchiq_complete_bulk(struct vchiq_bulk *bulk);
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_ioctl.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_ioctl.h
index 3653fd99d8a1..86d77f2eeea5 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_ioctl.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_ioctl.h
@@ -10,6 +10,17 @@ 
 #define VCHIQ_IOC_MAGIC 0xc4
 #define VCHIQ_INVALID_HANDLE (~0)
 
+struct vchiq_service_params {
+	int fourcc;
+	enum vchiq_status __user (*callback)(enum vchiq_reason reason,
+				      struct vchiq_header *header,
+				      unsigned int handle,
+				      void *bulk_userdata);
+	void __user *userdata;
+	short version;       /* Increment for non-trivial changes */
+	short version_min;   /* Update for incompatible changes */
+};
+
 struct vchiq_create_service {
 	struct vchiq_service_params params;
 	int is_open;
@@ -25,32 +36,32 @@  struct vchiq_queue_message {
 
 struct vchiq_queue_bulk_transfer {
 	unsigned int handle;
-	void *data;
+	void __user *data;
 	unsigned int size;
-	void *userdata;
+	void __user *userdata;
 	enum vchiq_bulk_mode mode;
 };
 
 struct vchiq_completion_data {
 	enum vchiq_reason reason;
-	struct vchiq_header *header;
-	void *service_userdata;
-	void *bulk_userdata;
+	struct vchiq_header __user *header;
+	void __user *service_userdata;
+	void __user *bulk_userdata;
 };
 
 struct vchiq_await_completion {
 	unsigned int count;
-	struct vchiq_completion_data *buf;
+	struct vchiq_completion_data __user *buf;
 	unsigned int msgbufsize;
 	unsigned int msgbufcount; /* IN/OUT */
-	void **msgbufs;
+	void * __user *msgbufs;
 };
 
 struct vchiq_dequeue_message {
 	unsigned int handle;
 	int blocking;
 	unsigned int bufsize;
-	void *buf;
+	void __user *buf;
 };
 
 struct vchiq_get_config {
@@ -65,7 +76,7 @@  struct vchiq_set_service_option {
 };
 
 struct vchiq_dump_mem {
-	void     *virt_addr;
+	void     __user *virt_addr;
 	size_t    num_bytes;
 };