Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1416783 > unrolled thread

Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher  interface Mat Martineau <mathew.j.martineau@linux.intel.com> - 2016-06-08 02:30 +0200
    Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface Stephan Mueller <smueller@chronox.de> - 2016-06-08 07:40 +0200
      Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher  interface Mat Martineau <mathew.j.martineau@linux.intel.com> - 2016-06-08 21:20 +0200
        Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface Stephan Mueller <smueller@chronox.de> - 2016-06-09 11:30 +0200
          Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher  interface Mat Martineau <mathew.j.martineau@linux.intel.com> - 2016-06-09 20:20 +0200
            Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface Stephan Mueller <smueller@chronox.de> - 2016-06-09 20:30 +0200
              Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher  interface Mat Martineau <mathew.j.martineau@linux.intel.com> - 2016-06-09 20:30 +0200
                Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface Stephan Mueller <smueller@chronox.de> - 2016-06-09 20:40 +0200
                  Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface Tadeusz Struk <tadeusz.struk@intel.com> - 2016-06-10 16:50 +0200

#1416783 — Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface

Stephan,

On Sat, 14 May 2016, Tadeusz Struk wrote:

> From: Stephan Mueller <smueller@chronox.de>
>
> This patch adds the user space interface for asymmetric ciphers. The
> interface allows the use of sendmsg as well as vmsplice to provide data.
>
> This version has been rebased on top of 4.6 and a few chackpatch issues
> have been fixed.
>
> Signed-off-by: Stephan Mueller <smueller@chronox.de>
> Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>
> ---
> diff --git a/crypto/algif_akcipher.c b/crypto/algif_akcipher.c
> new file mode 100644
> index 0000000..6342b6e
> --- /dev/null
> +++ b/crypto/algif_akcipher.c
> +
> +static int akcipher_recvmsg(struct socket *sock, struct msghdr *msg,
> +			    size_t ignored, int flags)
> +{
> +	struct sock *sk = sock->sk;
> +	struct alg_sock *ask = alg_sk(sk);
> +	struct akcipher_ctx *ctx = ask->private;
> +	struct akcipher_sg_list *sgl = &ctx->tsgl;
> +	unsigned int i = 0;
> +	int err;
> +	unsigned long used = 0;
> +	size_t usedpages = 0;
> +	unsigned int cnt = 0;
> +
> +	/* Limit number of IOV blocks to be accessed below */
> +	if (msg->msg_iter.nr_segs > ALG_MAX_PAGES)
> +		return -ENOMSG;
> +
> +	lock_sock(sk);
> +
> +	if (ctx->more) {
> +		err = akcipher_wait_for_data(sk, flags);
> +		if (err)
> +			goto unlock;
> +	}
> +
> +	used = ctx->used;
> +
> +	/* convert iovecs of output buffers into scatterlists */
> +	while (iov_iter_count(&msg->msg_iter)) {
> +		/* make one iovec available as scatterlist */
> +		err = af_alg_make_sg(&ctx->rsgl[cnt], &msg->msg_iter,
> +				     iov_iter_count(&msg->msg_iter));
> +		if (err < 0)
> +			goto unlock;
> +		usedpages += err;
> +		/* chain the new scatterlist with previous one */
> +		if (cnt)
> +			af_alg_link_sg(&ctx->rsgl[cnt - 1], &ctx->rsgl[cnt]);
> +
> +		iov_iter_advance(&msg->msg_iter, err);
> +		cnt++;
> +	}
> +
> +	/* ensure output buffer is sufficiently large */
> +	if (usedpages < akcipher_calcsize(ctx)) {
> +		err = -EMSGSIZE;
> +		goto unlock;
> +	}

Why is the size of the output buffer enforced here instead of depending on 
the algorithm implementation?

Thanks,

Mat


> +	sg_mark_end(sgl->sg + sgl->cur - 1);
> +
> +	akcipher_request_set_crypt(&ctx->req, sgl->sg, ctx->rsgl[0].sg, used,
> +				   usedpages);
> +	switch (ctx->op) {
> +	case ALG_OP_VERIFY:
> +		err = crypto_akcipher_verify(&ctx->req);
> +		break;
> +	case ALG_OP_SIGN:
> +		err = crypto_akcipher_sign(&ctx->req);
> +		break;
> +	case ALG_OP_ENCRYPT:
> +		err = crypto_akcipher_encrypt(&ctx->req);
> +		break;
> +	case ALG_OP_DECRYPT:
> +		err = crypto_akcipher_decrypt(&ctx->req);
> +		break;
> +	default:
> +		err = -EFAULT;
> +		goto unlock;
> +	}
> +
> +	err = af_alg_wait_for_completion(err, &ctx->completion);
> +
> +	if (err) {
> +		/* EBADMSG implies a valid cipher operation took place */
> +		if (err == -EBADMSG)
> +			akcipher_put_sgl(sk);
> +		goto unlock;
> +	}
> +
> +	akcipher_put_sgl(sk);
> +
> +unlock:
> +	for (i = 0; i < cnt; i++)
> +		af_alg_free_sg(&ctx->rsgl[i]);
> +
> +	akcipher_wmem_wakeup(sk);
> +	release_sock(sk);
> +
> +	return err ? err : ctx->req.dst_len;
> +}

--
Mat Martineau
Intel OTC

[toc] | [next] | [standalone]

#1416878 — Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface

Am Dienstag, 7. Juni 2016, 17:28:07 schrieb Mat Martineau:

Hi Mat,

> > +	used = ctx->used;
> > +
> > +	/* convert iovecs of output buffers into scatterlists */
> > +	while (iov_iter_count(&msg->msg_iter)) {
> > +		/* make one iovec available as scatterlist */
> > +		err = af_alg_make_sg(&ctx->rsgl[cnt], &msg->msg_iter,
> > +				     iov_iter_count(&msg->msg_iter));
> > +		if (err < 0)
> > +			goto unlock;
> > +		usedpages += err;
> > +		/* chain the new scatterlist with previous one */
> > +		if (cnt)
> > +			af_alg_link_sg(&ctx->rsgl[cnt - 1], &ctx->rsgl[cnt]);
> > +
> > +		iov_iter_advance(&msg->msg_iter, err);
> > +		cnt++;
> > +	}
> > +
> > +	/* ensure output buffer is sufficiently large */
> > +	if (usedpages < akcipher_calcsize(ctx)) {
> > +		err = -EMSGSIZE;
> > +		goto unlock;
> > +	}
> 
> Why is the size of the output buffer enforced here instead of depending on
> the algorithm implementation?

akcipher_calcsize calls crypto_akcipher_maxsize to get the maximum size the 
algorithm generates as output during its operation.

The code ensures that the caller provided at least that amount of memory for 
the kernel to store its data in. This check therefore is present to ensure the 
kernel does not overstep memory boundaries in user space.

What is your concern?

Thanks

Ciao
Stephan

[toc] | [prev] | [next] | [standalone]

#1417784

On Wed, 8 Jun 2016, Stephan Mueller wrote:

> Am Dienstag, 7. Juni 2016, 17:28:07 schrieb Mat Martineau:
>
> Hi Mat,
>
>>> +	used = ctx->used;
>>> +
>>> +	/* convert iovecs of output buffers into scatterlists */
>>> +	while (iov_iter_count(&msg->msg_iter)) {
>>> +		/* make one iovec available as scatterlist */
>>> +		err = af_alg_make_sg(&ctx->rsgl[cnt], &msg->msg_iter,
>>> +				     iov_iter_count(&msg->msg_iter));
>>> +		if (err < 0)
>>> +			goto unlock;
>>> +		usedpages += err;
>>> +		/* chain the new scatterlist with previous one */
>>> +		if (cnt)
>>> +			af_alg_link_sg(&ctx->rsgl[cnt - 1], &ctx->rsgl[cnt]);
>>> +
>>> +		iov_iter_advance(&msg->msg_iter, err);
>>> +		cnt++;
>>> +	}
>>> +
>>> +	/* ensure output buffer is sufficiently large */
>>> +	if (usedpages < akcipher_calcsize(ctx)) {
>>> +		err = -EMSGSIZE;
>>> +		goto unlock;
>>> +	}
>>
>> Why is the size of the output buffer enforced here instead of depending on
>> the algorithm implementation?
>
> akcipher_calcsize calls crypto_akcipher_maxsize to get the maximum size the
> algorithm generates as output during its operation.
>
> The code ensures that the caller provided at least that amount of memory for
> the kernel to store its data in. This check therefore is present to ensure the
> kernel does not overstep memory boundaries in user space.

Yes, it's understood that the userspace buffer length must not be 
exceeded. But dst_len is part of the akcipher_request struct, so why does 
it need to be checked *here* when it is also checked later?

> What is your concern?

Userspace must allocate larger buffers than it knows are necessary for 
expected results.

It looks like the software rsa implementation handles shorter output 
buffers ok (mpi_write_to_sgl will return EOVERFLOW if the the buffer is 
too small), however I see at least one hardware rsa driver that requires 
the output buffer to be the maximum size. But this inconsistency might be 
best addressed within the software cipher or drivers rather than in 
recvmsg.

--
Mat Martineau
Intel OTC

[toc] | [prev] | [next] | [standalone]

#1418191 — Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface

Am Mittwoch, 8. Juni 2016, 12:14:49 schrieb Mat Martineau:

Hi Mat,

> On Wed, 8 Jun 2016, Stephan Mueller wrote:
> > Am Dienstag, 7. Juni 2016, 17:28:07 schrieb Mat Martineau:
> > 
> > Hi Mat,
> > 
> >>> +	used = ctx->used;
> >>> +
> >>> +	/* convert iovecs of output buffers into scatterlists */
> >>> +	while (iov_iter_count(&msg->msg_iter)) {
> >>> +		/* make one iovec available as scatterlist */
> >>> +		err = af_alg_make_sg(&ctx->rsgl[cnt], &msg->msg_iter,
> >>> +				     iov_iter_count(&msg->msg_iter));
> >>> +		if (err < 0)
> >>> +			goto unlock;
> >>> +		usedpages += err;
> >>> +		/* chain the new scatterlist with previous one */
> >>> +		if (cnt)
> >>> +			af_alg_link_sg(&ctx->rsgl[cnt - 1], &ctx->rsgl[cnt]);
> >>> +
> >>> +		iov_iter_advance(&msg->msg_iter, err);
> >>> +		cnt++;
> >>> +	}
> >>> +
> >>> +	/* ensure output buffer is sufficiently large */
> >>> +	if (usedpages < akcipher_calcsize(ctx)) {
> >>> +		err = -EMSGSIZE;
> >>> +		goto unlock;
> >>> +	}
> >> 
> >> Why is the size of the output buffer enforced here instead of depending
> >> on
> >> the algorithm implementation?
> > 
> > akcipher_calcsize calls crypto_akcipher_maxsize to get the maximum size
> > the
> > algorithm generates as output during its operation.
> > 
> > The code ensures that the caller provided at least that amount of memory
> > for the kernel to store its data in. This check therefore is present to
> > ensure the kernel does not overstep memory boundaries in user space.
> 
> Yes, it's understood that the userspace buffer length must not be
> exceeded. But dst_len is part of the akcipher_request struct, so why does
> it need to be checked *here* when it is also checked later?

I am always uneasy when the kernel has a user space interface and expects 
layers deep down inside the kernel to check for user space related boundaries. 
Note, we do not hand the __user flag down, so sparse and other tools cannot 
detect whether a particular cipher implementation has the right checks.

I therefore always would like to check parameters at the interface handling 
logic. Cryptographers rightly should worry about their code implementing the 
cipher correctly. But I do not think that the cipher implementations should 
worry about security implications since they may be called from user space.
> 
> > What is your concern?
> 
> Userspace must allocate larger buffers than it knows are necessary for
> expected results.
> 
> It looks like the software rsa implementation handles shorter output
> buffers ok (mpi_write_to_sgl will return EOVERFLOW if the the buffer is
> too small), however I see at least one hardware rsa driver that requires
> the output buffer to be the maximum size. But this inconsistency might be
> best addressed within the software cipher or drivers rather than in
> recvmsg.

Is your concern that we have a double check check for lengths here? If yes, I 
think we can live with an additional if() here.

Or is your concern that the user space interface restricts things too much and 
thus prevents a valid use case?

Ciao
Stephan

[toc] | [prev] | [next] | [standalone]

#1418569

On Thu, 9 Jun 2016, Stephan Mueller wrote:

> Am Mittwoch, 8. Juni 2016, 12:14:49 schrieb Mat Martineau:
>
> Hi Mat,
>
>> On Wed, 8 Jun 2016, Stephan Mueller wrote:
>>> Am Dienstag, 7. Juni 2016, 17:28:07 schrieb Mat Martineau:
>>>
>>> Hi Mat,
>>>
>>>>> +	used = ctx->used;
>>>>> +
>>>>> +	/* convert iovecs of output buffers into scatterlists */
>>>>> +	while (iov_iter_count(&msg->msg_iter)) {
>>>>> +		/* make one iovec available as scatterlist */
>>>>> +		err = af_alg_make_sg(&ctx->rsgl[cnt], &msg->msg_iter,
>>>>> +				     iov_iter_count(&msg->msg_iter));
>>>>> +		if (err < 0)
>>>>> +			goto unlock;
>>>>> +		usedpages += err;
>>>>> +		/* chain the new scatterlist with previous one */
>>>>> +		if (cnt)
>>>>> +			af_alg_link_sg(&ctx->rsgl[cnt - 1], &ctx->rsgl[cnt]);
>>>>> +
>>>>> +		iov_iter_advance(&msg->msg_iter, err);
>>>>> +		cnt++;
>>>>> +	}
>>>>> +
>>>>> +	/* ensure output buffer is sufficiently large */
>>>>> +	if (usedpages < akcipher_calcsize(ctx)) {
>>>>> +		err = -EMSGSIZE;
>>>>> +		goto unlock;
>>>>> +	}
>>>>
>>>> Why is the size of the output buffer enforced here instead of 
>>>> depending on the algorithm implementation?
>>>
>>> akcipher_calcsize calls crypto_akcipher_maxsize to get the maximum 
>>> size the algorithm generates as output during its operation.
>>>
>>> The code ensures that the caller provided at least that amount of memory
>>> for the kernel to store its data in. This check therefore is present to
>>> ensure the kernel does not overstep memory boundaries in user space.
>>
>> Yes, it's understood that the userspace buffer length must not be
>> exceeded. But dst_len is part of the akcipher_request struct, so why does
>> it need to be checked *here* when it is also checked later?
>
> I am always uneasy when the kernel has a user space interface and expects
> layers deep down inside the kernel to check for user space related boundaries.
> Note, we do not hand the __user flag down, so sparse and other tools cannot
> detect whether a particular cipher implementation has the right checks.
>
> I therefore always would like to check parameters at the interface handling
> logic. Cryptographers rightly should worry about their code implementing the
> cipher correctly. But I do not think that the cipher implementations should
> worry about security implications since they may be called from user space.

Userspace or not, buffer lengths need to be strictly checked.

>>
>>> What is your concern?
>>
>> Userspace must allocate larger buffers than it knows are necessary for
>> expected results.
>>
>> It looks like the software rsa implementation handles shorter output
>> buffers ok (mpi_write_to_sgl will return EOVERFLOW if the the buffer is
>> too small), however I see at least one hardware rsa driver that requires
>> the output buffer to be the maximum size. But this inconsistency might be
>> best addressed within the software cipher or drivers rather than in
>> recvmsg.
>
> Is your concern that we have a double check check for lengths here? If yes, I
> think we can live with an additional if() here.
>
> Or is your concern that the user space interface restricts things too much and
> thus prevents a valid use case?

The latter - my primary concern is the constraint this places on userspace 
by forcing larger buffer sizes than might be necessary for the operation. 
struct akcipher_request has separate members for src_len and dst_len, and 
dst_len is documented as needing "to be at least as big as the expected 
result depending on the operation". Not the maximum result, the expected 
result. It's also documented that the cipher will generate an error if 
dst_len is insufficient and update the value with the required size.

I'm updating some userspace TLS code that worked with an earlier, unmerged 
patch set for AF_ALG akcipher (from last year). The read calls with 
shorter buffers were the main porting problem.

--
Mat Martineau
Intel OTC

[toc] | [prev] | [next] | [standalone]

#1418578 — Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface

Am Donnerstag, 9. Juni 2016, 11:18:04 schrieb Mat Martineau:

Hi Mat,

> > Or is your concern that the user space interface restricts things too much
> > and thus prevents a valid use case?
> 
> The latter - my primary concern is the constraint this places on userspace
> by forcing larger buffer sizes than might be necessary for the operation.
> struct akcipher_request has separate members for src_len and dst_len, and
> dst_len is documented as needing "to be at least as big as the expected
> result depending on the operation". Not the maximum result, the expected
> result. It's also documented that the cipher will generate an error if
> dst_len is insufficient and update the value with the required size.
> 
> I'm updating some userspace TLS code that worked with an earlier, unmerged
> patch set for AF_ALG akcipher (from last year). The read calls with
> shorter buffers were the main porting problem.

I see -- are you proposing to drop that check entirely?

Ciao
Stephan

[toc] | [prev] | [next] | [standalone]

#1418581

On Thu, 9 Jun 2016, Stephan Mueller wrote:

> Am Donnerstag, 9. Juni 2016, 11:18:04 schrieb Mat Martineau:
>
> Hi Mat,
>
>>> Or is your concern that the user space interface restricts things too much
>>> and thus prevents a valid use case?
>>
>> The latter - my primary concern is the constraint this places on userspace
>> by forcing larger buffer sizes than might be necessary for the operation.
>> struct akcipher_request has separate members for src_len and dst_len, and
>> dst_len is documented as needing "to be at least as big as the expected
>> result depending on the operation". Not the maximum result, the expected
>> result. It's also documented that the cipher will generate an error if
>> dst_len is insufficient and update the value with the required size.
>>
>> I'm updating some userspace TLS code that worked with an earlier, unmerged
>> patch set for AF_ALG akcipher (from last year). The read calls with
>> shorter buffers were the main porting problem.
>
> I see -- are you proposing to drop that check entirely?

Yes.


Best regards,

--
Mat Martineau
Intel OTC

[toc] | [prev] | [next] | [standalone]

#1418583 — Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface

Am Donnerstag, 9. Juni 2016, 11:27:13 schrieb Mat Martineau:

Hi Mat, Tadeusz,

> On Thu, 9 Jun 2016, Stephan Mueller wrote:
> > Am Donnerstag, 9. Juni 2016, 11:18:04 schrieb Mat Martineau:
> > 
> > Hi Mat,
> > 
> >>> Or is your concern that the user space interface restricts things too
> >>> much
> >>> and thus prevents a valid use case?
> >> 
> >> The latter - my primary concern is the constraint this places on
> >> userspace
> >> by forcing larger buffer sizes than might be necessary for the operation.
> >> struct akcipher_request has separate members for src_len and dst_len, and
> >> dst_len is documented as needing "to be at least as big as the expected
> >> result depending on the operation". Not the maximum result, the expected
> >> result. It's also documented that the cipher will generate an error if
> >> dst_len is insufficient and update the value with the required size.
> >> 
> >> I'm updating some userspace TLS code that worked with an earlier,
> >> unmerged
> >> patch set for AF_ALG akcipher (from last year). The read calls with
> >> shorter buffers were the main porting problem.
> > 
> > I see -- are you proposing to drop that check entirely?
> 
> Yes.

Ok, after checking the code again, I think that dropping that sanity check 
should be ok given that this length is part of the akcipher API.

Tadeusz, as you are currently managing that patch set, would you re-spin it 
with the following check removed?

+     if (usedpages < akcipher_calcsize(ctx)) {
+             err = -EMSGSIZE;
+             goto unlock;
+     }


Ciao
Stephan

[toc] | [prev] | [next] | [standalone]

#1419489 — Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface

On 06/09/2016 11:36 AM, Stephan Mueller wrote:
> Am Donnerstag, 9. Juni 2016, 11:27:13 schrieb Mat Martineau:
> 
> Hi Mat, Tadeusz,
> 
>> On Thu, 9 Jun 2016, Stephan Mueller wrote:
>>> Am Donnerstag, 9. Juni 2016, 11:18:04 schrieb Mat Martineau:
>>>
>>> Hi Mat,
>>>
>>>>> Or is your concern that the user space interface restricts things too
>>>>> much
>>>>> and thus prevents a valid use case?
>>>>
>>>> The latter - my primary concern is the constraint this places on
>>>> userspace
>>>> by forcing larger buffer sizes than might be necessary for the operation.
>>>> struct akcipher_request has separate members for src_len and dst_len, and
>>>> dst_len is documented as needing "to be at least as big as the expected
>>>> result depending on the operation". Not the maximum result, the expected
>>>> result. It's also documented that the cipher will generate an error if
>>>> dst_len is insufficient and update the value with the required size.
>>>>
>>>> I'm updating some userspace TLS code that worked with an earlier,
>>>> unmerged
>>>> patch set for AF_ALG akcipher (from last year). The read calls with
>>>> shorter buffers were the main porting problem.
>>>
>>> I see -- are you proposing to drop that check entirely?
>>
>> Yes.
> 
> Ok, after checking the code again, I think that dropping that sanity check 
> should be ok given that this length is part of the akcipher API.
> 
> Tadeusz, as you are currently managing that patch set, would you re-spin it 
> with the following check removed?
> 
> +     if (usedpages < akcipher_calcsize(ctx)) {
> +             err = -EMSGSIZE;
> +             goto unlock;
> +     }
> 

Ok, I'll update the patch.
Thanks,
-- 
TS

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web