Re: [GIT] More security subsystem fixes

[Kasatkin, Dmitry]

On Thu, Jan 19, 2012 at 4:21 PM, Tetsuo Handa
<penguin-kernel@xxxxxxxxxxxxxxxxxxx> wrote:
> Tetsuo Handa wrote:
>> James Morris wrote:
>> > Â Â Â MPILIB: Add a missing ENOMEM check
>> Maybe one more (shown below) with some random comments.
>
> Looked a bit more.
>
>
>
> In lib/mpi/mpih-div.c:
>
> mpi_limb_t
> mpihelp_divrem(mpi_ptr_t qp, mpi_size_t qextra_limbs,
> Â Â Â Â Â Â Â mpi_ptr_t np, mpi_size_t nsize, mpi_ptr_t dp, mpi_size_t dsize)
> {
> Â Â Â Âmpi_limb_t most_significant_q_limb = 0;
>
> Â Â Â Âswitch (dsize) {
> Â Â Â Âcase 0:
> Â Â Â Â Â Â Â Â/* We are asked to divide by zero, so go ahead and do it! Â(To make
> Â Â Â Â Â Â Â Â Â the compiler not remove this statement, return the value.) Â*/
> Â Â Â Â Â Â Â Âreturn 1 / dsize;
>
> What's this? Division by 0 in the kernel is no good.
>
>
>
> In lib/mpi/mpicoder.c:
>
> MPI do_encode_md(const void *sha_buffer, unsigned nbits)
> {
> (...snipped...)
> Â Â Â Ân = 0;
> Â Â Â Âframe[n++] = 0;
> Â Â Â Âframe[n++] = 1; Â Â Â Â /* block type */
> Â Â Â Âi = nframe - SHA1_DIGEST_LENGTH - asnlen - 3;
>
> Â Â Â Âif (i <= 1) {
> Â Â Â Â Â Â Â Âpr_info("MPI: message digest encoding failed\n");
> Â Â Â Â Â Â Â Âkfree(frame);
> Â Â Â Â Â Â Â Âreturn a;
> Â Â Â Â}
>
> Â Â Â Âmemset(frame + n, 0xff, i);
> Â Â Â Ân += i;
> Â Â Â Âframe[n++] = 0;
> Â Â Â Âmemcpy(frame + n, &asn, asnlen);
> Â Â Â Ân += asnlen;
> Â Â Â Âmemcpy(frame + n, sha_buffer, SHA1_DIGEST_LENGTH);
> Â Â Â Ân += SHA1_DIGEST_LENGTH;
>
> Â Â Â Âi = nframe;
> Â Â Â Âfr_pt = frame;
>
> Â Â Â Âif (n != nframe) {
>
> What's this? i = nframe - SHA1_DIGEST_LENGTH - asnlen - 3; equals
> nframe = i + SHA1_DIGEST_LENGTH + asnlen + 3; and this should be always true.

In fact n always equals to nframe, and this check is always false...


> Also, i = nframe; seems to make no sense because i is no longer used.
>
> Â Â Â Â Â Â Â Âprintk
> Â Â Â Â Â Â Â Â Â Â("MPI: message digest encoding failed, frame length is wrong\n");
> Â Â Â Â Â Â Â Âkfree(frame);
> Â Â Â Â Â Â Â Âreturn a;
> Â Â Â Â}
>
> Â Â Â Âa = mpi_alloc((nframe + BYTES_PER_MPI_LIMB - 1) / BYTES_PER_MPI_LIMB);
>
> Missing a != NULL check.
>
> Â Â Â Âmpi_set_buffer(a, frame, nframe, 0);
> Â Â Â Âkfree(frame);
>
> Â Â Â Âreturn a;
> }
>
>
>
> In lib/mpi/mpi-pow.c:
>
> int mpi_powm(MPI res, MPI base, MPI exp, MPI mod)
> {
> (...snipped...)
> Â Â Â Âif (!msize)
> Â Â Â Â Â Â Â Âmsize = 1 / msize; Â Â Â/* provoke a signal */
> (...snipped...)
> }
>
> Division by 0.
>
>
>
> In lib/mpi/mpi-div.c:
>
> int mpi_tdiv_q_2exp(MPI w, MPI u, unsigned count)
> {
> (...snipped...)
> Â Â Â Âusize = u->nlimbs;
> Â Â Â Âlimb_cnt = count / BITS_PER_MPI_LIMB;
> Â Â Â Âwsize = usize - limb_cnt;
> Â Â Â Âif (limb_cnt >= usize)
> Â Â Â Â Â Â Â Âw->nlimbs = 0;
> Â Â Â Âelse {
> Â Â Â Â Â Â Â Âmpi_ptr_t wp;
> Â Â Â Â Â Â Â Âmpi_ptr_t up;
>
> Â Â Â Â Â Â Â Âif (RESIZE_IF_NEEDED(w, wsize) < 0)
> Â Â Â Â Â Â Â Â Â Â Â Âreturn -ENOMEM;
> Â Â Â Â Â Â Â Âwp = w->d;
> Â Â Â Â Â Â Â Âup = u->d;
>
> Â Â Â Â Â Â Â Âcount %= BITS_PER_MPI_LIMB;
> Â Â Â Â Â Â Â Âif (count) {
> Â Â Â Â Â Â Â Â Â Â Â Âmpihelp_rshift(wp, up + limb_cnt, wsize, count);
> Â Â Â Â Â Â Â Â Â Â Â Âwsize -= !wp[wsize - 1];
> (...snipped...)
> }
>
> Is wsize > 0 guaranteed?
>
Should be, because of the check:
 Â Â Â Âif (limb_cnt >= usize)
 Â Â Â Â Â Â Â Âw->nlimbs = 0;
       else...


>
>
> Fixes (if any) will be needed to RHEL6's 2.6.32-220.2.1.el6 kernel as well.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at Âhttp://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/