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;
From:   Maximilian Luz <luzmaximilian@gmail.com>
To:     platform-driver-x86@vger.kernel.org
Cc:     Maximilian Luz <luzmaximilian@gmail.com>,
        Hans de Goede <hdegoede@redhat.com>,
        Mark Gross <mgross@linux.intel.com>,
        Colin Ian King <colin.king@canonical.com>,
        linux-kernel@vger.kernel.org
Subject: [PATCH -next 2/2] platform/surface: aggregator_cdev: Add comments regarding unchecked allocation size
Date:   Mon, 11 Jan 2021 16:48:51 +0100
Message-Id: <20210111154851.325404-3-luzmaximilian@gmail.com>
In-Reply-To: <20210111154851.325404-1-luzmaximilian@gmail.com>
References: <20210111154851.325404-1-luzmaximilian@gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

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