Don't modify the destination pointer in ioctl requests.

[jhb]

Summary

This breaks the case where the original pointer was NULL but an
in-line IV was used.

[jhb]

Arguably we shouldn't really be checking for NULL pointers to control copyout as if you request an operation that needs to write output and submit a NULL pointer you should get EFAULT rather than success but the output data is lost. That is, we should be writing to the output always for operations that generate output (everything but CSP_MODE_DIGEST I think which only writes to the mac pointer).

We should perhaps reject request with EINVAL if they pass in non-NULL pointers for fields that should be NULL as well (e.g. dst for a digest operation should be NULL, and the iv should be NULL for operations that don't use an iv).

[markj]

sys/opencrypto/cryptodev.c

1266 ↗

(On Diff #79094)

Is this the only reason caead can't be a pointer to const?

[jhb]

sys/opencrypto/cryptodev.c

1266 ↗

(On Diff #79094)

Yes. I could perhaps create an 'ivlen' local variable, but having the structures const isn't really required.

[markj]

Sorry, missed this comment earlier.

In D27064#603981, @jhb wrote:

Arguably we shouldn't really be checking for NULL pointers to control copyout as if you request an operation that needs to write output and submit a NULL pointer you should get EFAULT rather than success but the output data is lost. That is, we should be writing to the output always for operations that generate output (everything but CSP_MODE_DIGEST I think which only writes to the mac pointer).

I agree.

We should perhaps reject request with EINVAL if they pass in non-NULL pointers for fields that should be NULL as well (e.g. dst for a digest operation should be NULL, and the iv should be NULL for operations that don't use an iv).

That seems reasonable to me. The only argument against it is that it might break existing software. We could perhaps introduce new ioctls with stricter behaviour and deprecate the old ones.

[jhb]

In D27064#604646, @markj wrote:

Sorry, missed this comment earlier.

In D27064#603981, @jhb wrote:

Arguably we shouldn't really be checking for NULL pointers to control copyout as if you request an operation that needs to write output and submit a NULL pointer you should get EFAULT rather than success but the output data is lost. That is, we should be writing to the output always for operations that generate output (everything but CSP_MODE_DIGEST I think which only writes to the mac pointer).

I agree.

We should perhaps reject request with EINVAL if they pass in non-NULL pointers for fields that should be NULL as well (e.g. dst for a digest operation should be NULL, and the iv should be NULL for operations that don't use an iv).

That seems reasonable to me. The only argument against it is that it might break existing software. We could perhaps introduce new ioctls with stricter behaviour and deprecate the old ones.

I strongly doubt there is any real software using these ioctls. The openssl engine uses it, but it doesn't currently use it for much of anything. (I had old patches for 1.0.2 I need to rewrite someday to add AES-GCM support including the special sauce for AES-GCM when doing TLS.) The only other consumer is probably cryptocheck, and it should probably already be doing the right thing. There's not really a compelling reason to pass a buffer you know won't be used. I do think though that this type of change should be a separate followup.