Skip to content

Conversation

@vcsjones
Copy link

@vcsjones vcsjones commented Jul 4, 2025

This also adds error checking when creating the context.

This also adds error checking when creating the context.
}

// Required for OpenSSL 1.1 AES-KWP, no-op in OpenSSL 3.
EVP_CIPHER_CTX_set_flags(ctx, EVP_CIPHER_CTX_FLAG_WRAP_ALLOW);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this flag the error is

Interop+Crypto+OpenSslCryptographicException : error:0607B0AA:digital envelope routines:EVP_CipherInit_ex:wrap mode not allowed

This history of this flag is interesting. Originally, OpenSSL 1.1 started requiring this flag so that callers were sure their buffers were allocated correctly. AES-KWP doesn't fit neatly in to the "How much data am I going to write" allocation helpers, so this flag was introduced to help mitigate buffer overruns and callers had to explicitly indicate "Yes I know what I am doing and yes the buffers are correctly sized".

IntPtr algorithm = GetKeyWrapAlgorithm(keySizeInBits);

return Interop.Crypto.EvpCipherCreate(
SafeEvpCipherCtxHandle ctx = Interop.Crypto.EvpCipherCreate(
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of these days we should make all LibraryImport's private and the internal ones do the error checking like we have recently started doing.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, agreed. One day.

ref MemoryMarshal.GetReference(ReadOnlySpan<byte>.Empty),
enc);

if (ctx.IsInvalid)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did originally have a call to Interop.Crypto.CheckValidHandle (or whatever it is), but I clearly lost it during refactoring. I would say "good catch", but since you got here via debugging crashes, I'll just say "whoops!"

@bartonjs bartonjs merged commit e2fb2fa into bartonjs:aeskw_ossl Jul 7, 2025
@vcsjones vcsjones deleted the aeskw_ossl_fix11 branch July 7, 2025 16:54
bartonjs pushed a commit that referenced this pull request Sep 19, 2025
…et#112740)

Recent work now allows us to finally add support for the backend to
extract fields out of parameters without spilling them to stack.
Previously this was only supported when the fields mapped cleanly to
registers.

A win-x64 example:
```csharp
static int Foo(int? foo)
{
    return foo.HasValue ? foo.Value : 0;
}
```
```diff
 ; Method Program:Foo(System.Nullable`1[int]):int (FullOpts)
 G_M19236_IG01:  ;; offset=0x0000
-       mov      qword ptr [rsp+0x08], rcx
-						;; size=5 bbWeight=0.50 PerfScore 0.50
+						;; size=0 bbWeight=0.50 PerfScore 0.00

-G_M19236_IG02:  ;; offset=0x0005
-       movzx    rcx, cl
-       xor      eax, eax
-       test     ecx, ecx
-       cmovne   eax, dword ptr [rsp+0x0C]
-						;; size=12 bbWeight=0.50 PerfScore 1.38
+G_M19236_IG02:  ;; offset=0x0000
+       movzx    rax, cl
+       shr      rcx, 32
+       xor      edx, edx
+       test     eax, eax
+       mov      eax, edx
+       cmovne   eax, ecx
+						;; size=16 bbWeight=0.50 PerfScore 0.88

-G_M19236_IG03:  ;; offset=0x0011
+G_M19236_IG03:  ;; offset=0x0010
        ret
 						;; size=1 bbWeight=0.50 PerfScore 0.50
-; Total bytes of code: 18
-
+; Total bytes of code: 17
```

Another win-x64 example:
```csharp
static float Sum(PointF p)
{
    return p.X + p.Y;
}
```

```diff
 ; Method Program:Sum(System.Drawing.PointF):float (FullOpts)
 G_M48891_IG01:  ;; offset=0x0000
-       mov      qword ptr [rsp+0x08], rcx
-						;; size=5 bbWeight=1 PerfScore 1.00
+						;; size=0 bbWeight=1 PerfScore 0.00

-G_M48891_IG02:  ;; offset=0x0005
-       vmovss   xmm0, dword ptr [rsp+0x08]
-       vaddss   xmm0, xmm0, dword ptr [rsp+0x0C]
-						;; size=12 bbWeight=1 PerfScore 8.00
+G_M48891_IG02:  ;; offset=0x0000
+       vmovd    xmm0, ecx
+       shr      rcx, 32
+       vmovd    xmm1, ecx
+       vaddss   xmm0, xmm0, xmm1
+						;; size=16 bbWeight=1 PerfScore 7.50

-G_M48891_IG03:  ;; offset=0x0011
+G_M48891_IG03:  ;; offset=0x0010
        ret
 						;; size=1 bbWeight=1 PerfScore 1.00
-; Total bytes of code: 18
+; Total bytes of code: 17
```

An arm64 example:
```csharp
static bool Test(Memory<int> mem)
{
    return mem.Length > 10;
}
```

```diff
 ; Method Program:Test(System.Memory`1[int]):ubyte (FullOpts)
 G_M53448_IG01:  ;; offset=0x0000
-            stp     fp, lr, [sp, #-0x20]!
+            stp     fp, lr, [sp, #-0x10]!
             mov     fp, sp
-            stp     x0, x1, [fp, #0x10]	// [V00 arg0], [V00 arg0+0x08]
-						;; size=12 bbWeight=1 PerfScore 2.50
+						;; size=8 bbWeight=1 PerfScore 1.50

-G_M53448_IG02:  ;; offset=0x000C
-            ldr     w0, [fp, #0x1C]	// [V00 arg0+0x0c]
+G_M53448_IG02:  ;; offset=0x0008
+            lsr     x0, x1, dotnet#32
             cmp     w0, #10
             cset    x0, gt
-						;; size=12 bbWeight=1 PerfScore 3.00
+						;; size=12 bbWeight=1 PerfScore 2.00

-G_M53448_IG03:  ;; offset=0x0018
-            ldp     fp, lr, [sp], #0x20
+G_M53448_IG03:  ;; offset=0x0014
+            ldp     fp, lr, [sp], #0x10
             ret     lr
 						;; size=8 bbWeight=1 PerfScore 2.00
-; Total bytes of code: 32
+; Total bytes of code: 28
```

Float -> float extractions that do not map cleanly is still not supported, but
should be doable (via vector register extractions). Float -> int extractions are
not supported, but I'm not sure we see these.

This is often not a code size improvement, but typically a perfscore
improvement. Also this seems to have some bad interactions with call arguments
since they do not yet support something similar, but hopefully that can be
improved separately.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants