Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 2 Jun 2021 01:35:13 +0530
From:   Ojaswin Mujoo <ojaswin98@gmail.com>
To:     nsaenz@kernel.org
Cc:     gregkh@linuxfoundation.org, arnd@arndb.de,
        dan.carpenter@oracle.com, phil@raspberrypi.com,
        bcm-kernel-feedback-list@broadcom.com,
        linux-arm-kernel@lists.infradead.org,
        linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: staging: vc04_services: Need suggestions on trying to fix sparse
 warning in vchiq_arm.c
Message-ID: <20210601200513.GA10204@ojas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Precedence: bulk

Hello,

I was trying to address the following TODO item in
"drivers/staging/vc04_services/interface/TODO" and would love some comments and
suggestions on this:

>  14) Clean up Sparse warnings from __user annotations. See
>  vchiq_irq_queue_bulk_tx_rx(). Ensure that the address of "&waiter->bulk_waiter"
>  is never disclosed to userspace.

More specifically, I was looking at ways to fix the following sparse warning:

	drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c:1001:26:

	warning: incorrect type in assignment (different address spaces)
	expected void *[assigned] userdata
	got void [noderef] __user *userdata

From my understanding, the issue seems to be that the (void *)userdata local
variable in vchiq_irq_queue_bulk_tx_rx() can be assigned a (void __user *) or a
(struct bulk_waiter *). This makes the assignment tricky since it can either be
a userspace pointer or a kernel pointer.

Right now, we are just ignoring the sparse warning which is essentially
resulting in the __user attribute being lost when we assign args->userdata to
userdata. This can be dangerous as it might result in illegal access of
userspace memory, without any sparse warning, down the line. 

Further, this issue seems to boil down to the fact that the (void *)userdata
field in struct vchiq_bulk (in vc04_services/interface/vchiq_arm/vchiq_core.h)
can, again, either be a (stuct bulk_waiter *) or a (void __user *). To fix this,
I was playing with the idea of modifying this userdata field of struct
vchiq_bulk to be something like the following:

	diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h 
	b/ drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h

	--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
	+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h

	@@ -227,10 +227,16 @@ enum vchiq_bulk_dir {
	 
	 typedef void (*vchiq_userdata_term)(void *userdata);
	 
	+struct vchiq_userdata_or_waiter {
	+		void __user *userdata;
	+		struct bulk_waiter *bulk_waiter;
	+               
	+};
	+
	 struct vchiq_bulk {
			short mode;
			short dir;
	-		void *userdata;
	+		struct vchiq_userdata_or_waiter *userdata_or_waiter;
			dma_addr_t data;
			int size;
			void *remote_data;

I was then planning on modifying all the code that works with
vchiq_bulk->userdata accordingly. I believe this can help us overcome the sparse
warnings, as well as preserve the __user attribute in cases when the userspace
pointer does get assigned here. 

However, since I'm not very familiar with the codebase, I just wanted to confirm
if this is an acceptable workaround and to make sure I'm not breaking anything
or overlooked anything here. I noticed that we also want to make sure that 
bulk_waiter's address is not exposed to userspace. Will it be possible to provide
some pointers on how/where this might happen, so I can see if I can try to extend 
this patch to avoid that.

I would love to hear you suggestions and thoughts on this.

PS: I'm new here so please do correct me incase I missed anything.

Regards,
Ojaswin