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
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
