On 8/15/2016 10:26 PM, Chris Wilson wrote:
On Mon, Aug 15, 2016 at 10:16:56PM +0530, Goel, Akash wrote:
On 8/15/2016 9:36 PM, Chris Wilson wrote:
On Mon, Aug 15, 2016 at 08:19:47PM +0530, akash.goel@xxxxxxxxx wrote:
+static void guc_read_update_log_buffer(struct intel_guc *guc)
+{
+ struct guc_log_buffer_state *log_buffer_state, *log_buffer_snapshot_state;
+ struct guc_log_buffer_state log_buffer_state_local;
+ void *src_data_ptr, *dst_data_ptr;
+ unsigned int buffer_size, expected_size;
+ enum guc_log_buffer_type type;
+
+ if (WARN_ON(!guc->log.buf_addr))
+ return;
+
+ /* Get the pointer to shared GuC log buffer */
+ log_buffer_state = src_data_ptr = guc->log.buf_addr;
+
+ /* Get the pointer to local buffer to store the logs */
+ dst_data_ptr = log_buffer_snapshot_state = guc_get_write_buffer(guc);
+
+ /* Actual logs are present from the 2nd page */
+ src_data_ptr += PAGE_SIZE;
+ dst_data_ptr += PAGE_SIZE;
+
+ for (type = GUC_ISR_LOG_BUFFER; type < GUC_MAX_LOG_BUFFER; type++) {
+ /* Make a copy of the state structure in GuC log buffer (which
+ * is uncached mapped) on the stack to avoid reading from it
+ * multiple times.
+ */
+ memcpy(&log_buffer_state_local, log_buffer_state,
+ sizeof(struct guc_log_buffer_state));
+ buffer_size = log_buffer_state_local.size;
+
+ if (log_buffer_snapshot_state) {
+ /* First copy the state structure in snapshot buffer */
+ memcpy(log_buffer_snapshot_state, &log_buffer_state_local,
+ sizeof(struct guc_log_buffer_state));
+
+ /* The write pointer could have been updated by the GuC
+ * firmware, after sending the flush interrupt to Host,
+ * for consistency set the write pointer value to same
+ * value of sampled_write_ptr in the snapshot buffer.
+ */
+ log_buffer_snapshot_state->write_ptr =
+ log_buffer_snapshot_state->sampled_write_ptr;
+
+ log_buffer_snapshot_state++;
+
+ /* Now copy the actual logs, but before that validate
+ * the buffer size value retrieved from state structure.
+ */
+ if (type == GUC_ISR_LOG_BUFFER)
+ expected_size = (GUC_LOG_ISR_PAGES+1)*PAGE_SIZE;
+ else if (type == GUC_DPC_LOG_BUFFER)
+ expected_size = (GUC_LOG_DPC_PAGES+1)*PAGE_SIZE;
+ else
+ expected_size = (GUC_LOG_CRASH_PAGES+1)*PAGE_SIZE;
+
+ if (unlikely(buffer_size != expected_size)) {
+ DRM_ERROR("unexpected log buffer size\n");
+ /* Continue with further copying, already state
+ * structure has been copied which is enough to
+ * let Userspace know about the anomaly.
+ */
+ buffer_size = expected_size;
Urm, no.
You tell userspace one thing and then do another. This code should just
be a conduit and not apply its own outdated interpretation.
Userspace parser would get to know from the state structure about
the anomalous buffer size.
It will, but it won't be told what the kernel did. So if believes the
GuC (as it should since it is a packet that should be unadulterated) the
entire stream is then corrupt.
Please suggest that what should be done here ideally.
Should the further copying (for this snapshot) be skipped ?
The kernel should be avoiding interpretting the log packets as much as
possible - I would prefer it if we just moved the byte stream without
trying to interpret it as datagrams. But there is probably some merit to
at least using the log packets (datagrams).
It would have been ideal if log packets can be dumped without any
interpretation.
We copy the payload without any interpretation, only some bits of header
we parse.
We also have to interpret the header (in subsequent patch) to copy only
the updated payload data, for better performance.
+ }
+
+ memcpy(dst_data_ptr, src_data_ptr, buffer_size);
Where do you validate that buffer_size is sane before copying?
Sorry didn't get you, the check for buffer_size is being done right
before this memcpy.
There is no explicit check for valid src_data_ptr + buffer_size or
dst_data_ptr + buffer_size, and a quick glance at the code suggested no
reason to believe they must be valid.
Actually if buffer_size has been validated & corrected, then both
src_data_ptr + buffer_size and dst_data_ptr + buffer_size should be
valid. Both buffers have been allocated by Driver.
Will avoid reading the buffer_size from the header and just move the
pointer & do the copy as per the expected buffer size, which cannot go
wrong (out of bounds) as Driver only allocated the log buffer.
Best regards
Akash
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
