[PATCH -next 0/2] platform/surface: aggregator_cdev: Fixes for CI analysis

[PATCH -next 0/2] platform/surface: aggregator_cdev: Fixes for CI analysis

[Maximilian Luz]

Hi,

here are some patches addressing two issues in the Surface Aggregator
user-space interface reported by Colin Ian King via static analysis.

Maximilian Luz (2):
  platform/surface: aggregator_cdev: Fix access of uninitialized
    variables
  platform/surface: aggregator_cdev: Add comments regarding unchecked
    allocation size

 .../surface/surface_aggregator_cdev.c         | 23 +++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

-- 
2.30.0

[PATCH -next 1/2] platform/surface: aggregator_cdev: Fix access of uninitialized variables

[Maximilian Luz]

When copy_struct_from_user() in ssam_cdev_request() fails, we directly
jump to the 'out' label. In this case, however 'spec' and 'rsp' are not
initialized, but we still access fields of those variables. Fix this by
initializing them at the time of their declaration.

Reported-by: Colin Ian King <colin.king@canonical.com>
Fixes: 178f6ab77e61 ("platform/surface: Add Surface Aggregator user-space interface")
Addresses-Coverity: ("Uninitialized pointer read")
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
---
 drivers/platform/surface/surface_aggregator_cdev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/surface/surface_aggregator_cdev.c b/drivers/platform/surface/surface_aggregator_cdev.c
index 340d15b148b9..979340cdd9de 100644
--- a/drivers/platform/surface/surface_aggregator_cdev.c
+++ b/drivers/platform/surface/surface_aggregator_cdev.c
@@ -66,8 +66,8 @@ static long ssam_cdev_request(struct ssam_cdev *cdev, unsigned long arg)
 {
 	struct ssam_cdev_request __user *r;
 	struct ssam_cdev_request rqst;
-	struct ssam_request spec;
-	struct ssam_response rsp;
+	struct ssam_request spec = {};
+	struct ssam_response rsp = {};
 	const void __user *plddata;
 	void __user *rspdata;
 	int status = 0, ret = 0, tmp;
-- 
2.30.0

[PATCH -next 2/2] platform/surface: aggregator_cdev: Add comments regarding unchecked allocation size

[Maximilian Luz]

CI static analysis complains about the allocation size in payload and
response buffers being unchecked. In general, these allocations should
be safe as the user-input is u16 and thus limited to U16_MAX, which is
only slightly larger than the theoretical maximum imposed by the
underlying SSH protocol.

All bounds on these values required by the underlying protocol are
enforced in ssam_request_sync() (or rather the functions called by it),
thus bounds here are only relevant for allocation.

Add comments explaining that this should be safe.

Reported-by: Colin Ian King <colin.king@canonical.com>
Fixes: 178f6ab77e61 ("platform/surface: Add Surface Aggregator user-space interface")
Addresses-Coverity: ("Untrusted allocation size")
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
---
 .../surface/surface_aggregator_cdev.c         | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/platform/surface/surface_aggregator_cdev.c b/drivers/platform/surface/surface_aggregator_cdev.c
index 979340cdd9de..ccfffe5eadfc 100644
--- a/drivers/platform/surface/surface_aggregator_cdev.c
+++ b/drivers/platform/surface/surface_aggregator_cdev.c
@@ -106,6 +106,15 @@ static long ssam_cdev_request(struct ssam_cdev *cdev, unsigned long arg)
 			goto out;
 		}
 
+		/*
+		 * Note: spec.length is limited to U16_MAX bytes via struct
+		 * ssam_cdev_request. This is slightly larger than the
+		 * theoretical maximum (SSH_COMMAND_MAX_PAYLOAD_SIZE) of the
+		 * underlying protocol (note that nothing remotely this size
+		 * should ever be allocated in any normal case). This size is
+		 * validated later in ssam_request_sync(), for allocation the
+		 * bound imposed by u16 should be enough.
+		 */
 		spec.payload = kzalloc(spec.length, GFP_KERNEL);
 		if (!spec.payload) {
 			ret = -ENOMEM;
@@ -125,6 +134,16 @@ static long ssam_cdev_request(struct ssam_cdev *cdev, unsigned long arg)
 			goto out;
 		}
 
+		/*
+		 * Note: rsp.capacity is limited to U16_MAX bytes via struct
+		 * ssam_cdev_request. This is slightly larger than the
+		 * theoretical maximum (SSH_COMMAND_MAX_PAYLOAD_SIZE) of the
+		 * underlying protocol (note that nothing remotely this size
+		 * should ever be allocated in any normal case). In later use,
+		 * this capacity does not have to be strictly bounded, as it
+		 * is only used as an output buffer to be written to. For
+		 * allocation the bound imposed by u16 should be enough.
+		 */
 		rsp.pointer = kzalloc(rsp.capacity, GFP_KERNEL);
 		if (!rsp.pointer) {
 			ret = -ENOMEM;
-- 
2.30.0