From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: dma-buf: heaps: system: add an option to allocate
 explicitly decrypted memory
Date: Wed, 11 Feb 2026 16:59:18 +1000
Message-ID: <review-patch5-20260209153809.250835-6-jiri@resnulli.us>
In-Reply-To: <20260209153809.250835-6-jiri@resnulli.us>
References: <20260209153809.250835-1-jiri@resnulli.us> <20260209153809.250835-6-jiri@resnulli.us>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
X-Mailer: Claude Code Patch Reviewer

Patch Review

**Subject**: `dma-buf: heaps: system: add an option to allocate explicitly decrypted memory`  
**Files Modified**: `drivers/dma-buf/heaps/system_heap.c`, `include/linux/dma-heap.h`, `include/uapi/linux/dma-heap.h`  
**Verdict**: ⚠️ **Needs revision** (good concept, implementation issues)

#### Technical Review

Adds support for allocating decrypted memory from system heap using `DMA_HEAP_FLAG_DECRYPTED`.

**UAPI Addition** (include/uapi/linux/dma-heap.h:1018-1027):
```c
+/**
+ * DMA_HEAP_FLAG_DECRYPTED - Allocate decrypted (shared) memory
+ *
+ * For confidential computing guests (AMD SEV, Intel TDX), this flag
+ * requests that the allocated memory be marked as decrypted (shared
+ * with the host).
+ */
+#define DMA_HEAP_FLAG_DECRYPTED        (1ULL << 0)
```

**Critical Issues**:

#### 1. **Build Failures** ❌

>From kernel test robot:
```
s390 architecture:
>> drivers/dma-buf/heaps/system_heap.c:822:15: error: implicit declaration 
   of function 'set_memory_decrypted'
>> drivers/dma-buf/heaps/system_heap.c:838:15: error: implicit declaration 
   of function 'set_memory_encrypted'
```

**Root cause**: `set_memory_decrypted()`/`set_memory_encrypted()` not available on all architectures.

**Fix needed**: Add architecture guards or provide stubs. Example:
```c
#if defined(CONFIG_ARCH_HAS_SET_MEMORY_ENCRYPT) || defined(CONFIG_AMD_MEM_ENCRYPT)
    // Current implementation
#else
    if (decrypted)
        return ERR_PTR(-EOPNOTSUPP);
#endif
```

#### 2. **HIGHMEM Sanity Check** (lines 942-944):

```c
+#ifdef CONFIG_HIGHMEM
+   /* Sanity check, should not happen. */
+   return ERR_PTR(-EINVAL);
+#endif
```

**Issues**:
- ⚠️ Comment says "should not happen" but doesn't explain *why*
- ⚠️ Why is HIGHMEM incompatible? Is it because `set_memory_*()` requires direct-mapped memory?
- ⚠️ This check happens AFTER `cc_platform_has()` check, so on non-CC HIGHMEM systems we don't hit this
- ⚠️ Should this be `BUILD_BUG_ON()` instead if it truly can't happen?

**Missing**: Explanation in commit message about HIGHMEM incompatibility

#### 3. **Memory Leak on Re-encryption Failure** (lines 915-920, 977-983):

```c
+   /*
+    * Intentionally leak pages that cannot be re-encrypted
+    * to prevent decrypted memory from being reused.
+    */
+   if (buffer->decrypted &&
+       system_heap_set_page_encrypted(page))
+       continue;
```

**Analysis**:
- ✅ Security-critical: Leaking is correct behavior to prevent decrypted pages from entering general memory pool
- ✅ Warning message is appropriate (pr_warn_ratelimited)
- ⚠️ **Question**: Should this also trigger a more severe response? E.g., `WARN_ONCE()` or even system-wide flag that something is wrong?
- ⚠️ **Memory accounting**: Leaked pages will show as "lost" memory. Should there be a counter or trace event?

**Recommendation**: Consider adding:
```c
WARN_ONCE(1, "Failed to re-encrypt memory, page leaked\n");
trace_dma_heap_page_leak(page);
```

#### 4. **pgprot_decrypted() Usage** (lines 874-876, 904-906):

```c
+   prot = vma->vm_page_prot;
+   if (buffer->decrypted)
+       prot = pgprot_decrypted(prot);
```

**Questions**:
- Is `pgprot_decrypted()` available on all architectures?
- What does it do on non-CC architectures? (Should be no-op, but verify)
- Similar issue for vmap path

#### 5. **DMA Mapping** (lines 860-862):

```c
+   attrs = a->decrypted ? DMA_ATTR_CC_DECRYPTED : 0;
+   ret = dma_map_sgtable(attachment->dev, table, direction, attrs);
```

✅ Correct usage of `DMA_ATTR_CC_DECRYPTED` from PATCH 2  
✅ Flags propagated from buffer to attachment  

#### 6. **Runtime Check** (lines 939-940):

```c
+   if (decrypted) {
+       if (!cc_platform_has(CC_ATTR_MEM_ENCRYPT))
+           return ERR_PTR(-EOPNOTSUPP);
```

✅ **Correct**: Rejects flag on non-CC systems  
✅ **Security**: Prevents misuse by userspace that doesn't understand shared memory implications  

**Question from Jason Gunthorpe review**: "On CC VM systems shared/private is a universal property. It is security-dangerous if heaps start returning 'shared' memory without userspace knowing about it."

This raises a broader question: Should ALL existing heaps (CMA, etc.) be disabled on CC systems until audited?

#### 7. **Design Issue: Using Flags Instead of Separate Heap** ❌

As discussed in PATCH 4, this should be a separate heap name, not a flag. Per maintainer feedback and author agreement, v2 will use "system_cc_decrypted" heap name.

**Required changes for v2**:
- Remove flag-based approach
- Create new heap registration in `system_heap_create()`
- Possibly share code between normal and decrypted heaps

---

## ADDITIONAL CONCERNS

### Security Review

>From Jason Gunthorpe's feedback:

> "On CC VM systems shared/private is a universal property. Userspace should 
> positively signal via the API that it wants/accepts shared memory. It is 
> security-dangerous if heaps start returning 'shared' memory without 
> userspace knowing about it."

**Questions for maintainers**:
1. What is the state of OTHER heaps (CMA, protected, etc.) on CC systems?
2. Should there be a global policy to disable non-audited heaps on CC systems?
3. Should there be a kernel-wide "I accept shared memory risk" signal from userspace?

### API Evolution Concerns

>From Leon Romanovsky:

> "You can't base your user-visible APIs on name as it limits extend 
> abilities in the future."

**Counter-argument**: dma-buf heaps API is already name-based (`/dev/dma_heap/<name>`). But valid concern about extensibility.

### Testing

**Missing from patch series**:
- ❌ No selftests for new functionality
- ❌ No documentation updates
- ❌ No mention of testing on actual CC hardware (AMD SEV? Intel TDX?)

---

## SUMMARY AND RECOMMENDATIONS

### Blocking Issues
1. ❌ **PATCH 4 must be dropped** - violates design principles
2. ❌ **Build failures on s390** - must be fixed
3. ❌ **v2 needed** - change to separate heap approach per maintainer feedback

### Non-Blocking Issues Requiring Attention
1. ⚠️ PATCH 1: Consider keeping warning message (without bogus dma_addr)
2. ⚠️ PATCH 2: Document `phys_to_dma_unencrypted()` architecture availability
3. ⚠️ PATCH 5: Document HIGHMEM incompatibility
4. ⚠️ PATCH 5: Consider stronger warning on re-encryption failure
5. ⚠️ Series: Add selftests
6. ⚠️ Series: Add documentation

### Design Questions for v2
1. Should we share code between "system" and "system_cc_decrypted" heaps?
2. Should existing heaps be disabled on CC systems?
3. Should `pgprot_decrypted()` availability be checked at compile time?

### Positive Aspects
- ✅ Addresses real problem in CoCo ecosystem
- ✅ Core technical approach is sound (decryption, DMA mapping, page protection)
- ✅ Security-conscious (intentional leak on re-encryption failure, runtime checks)
- ✅ Author is responsive to feedback and willing to revise

**Overall**: Good idea, needs rework for v2 per maintainer guidance.

---
Generated by Claude Code Patch Reviewer