Received-SPF: pass (google.com: best guess record for domain of linux-crypto-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200126133805.20294-1-gilad@benyossef.com>
In-Reply-To: <20200126133805.20294-1-gilad@benyossef.com>
From:   Geert Uytterhoeven <geert@linux-m68k.org>
Date:   Mon, 27 Jan 2020 09:03:19 +0100
Message-ID: <CAMuHMdXHgMn8L2_CZ8kXcp3g4Y+3HfQsvFhyTZatf8-xk2kUdQ@mail.gmail.com>
Subject: Re: [RFC] crypto: ccree - protect against short scatterlists
To:     Gilad Ben-Yossef <gilad@benyossef.com>
Cc:     Herbert Xu <herbert@gondor.apana.org.au>,
        "David S. Miller" <davem@davemloft.net>,
        Ofir Drang <ofir.drang@arm.com>,
        Linux Crypto Mailing List <linux-crypto@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-crypto-owner@vger.kernel.org
Precedence: bulk

Hi Gilad,

On Sun, Jan 26, 2020 at 2:38 PM Gilad Ben-Yossef <gilad@benyossef.com> wrote:
> Deal gracefully with the event of being handed a scatterlist
> which is shorter than expected.
>
> This mitigates a crash in some cases of crashes due to
> attempt to map empty (but not NULL) scatterlists with none
> zero lengths.
>
> This is an interim patch, to help diagnoze the issue, not
> intended for mainline in its current form as of yet.
>
> Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>

Thanks for your patch!

Unfortunately this doesn't make a difference, as ...

> --- a/drivers/crypto/ccree/cc_buffer_mgr.c
> +++ b/drivers/crypto/ccree/cc_buffer_mgr.c
> @@ -286,10 +286,32 @@ static void cc_add_sg_entry(struct device *dev, struct buffer_array *sgl_data,
>         sgl_data->num_of_buffers++;
>  }
>
> +static unsigned int cc_sg_trunc_len(struct scatterlist *sg, unsigned int len)
> +{
> +       unsigned int total;
> +
> +       if (!len)
> +               return 0;
> +
> +       for (total = 0; sg; sg = sg_next(sg)) {
> +               total += sg->length;
> +               if (total >= len) {
> +                       total = len;
> +                       break;
> +               }
> +       }
> +
> +       return total;
> +}
> +
>  static int cc_map_sg(struct device *dev, struct scatterlist *sg,
>                      unsigned int nbytes, int direction, u32 *nents,
>                      u32 max_sg_nents, u32 *lbytes, u32 *mapped_nents)
>  {
> +       int ret;
> +
> +       nbytes = cc_sg_trunc_len(sg, nbytes);
> +
>         if (sg_is_last(sg)) {

(1) this branch is taken, and not the else below,
(2) nothing acts upon detecting nbytes = 0.

With extra debug print:

    cc_map_sg: nbytes  = 0, first branch taken

>                 /* One entry only case -set to DLLI */
>                 if (dma_map_sg(dev, sg, 1, direction) != 1) {
> @@ -313,12 +335,14 @@ static int cc_map_sg(struct device *dev, struct scatterlist *sg,
>                 /* In case of mmu the number of mapped nents might
>                  * be changed from the original sgl nents
>                  */
> -               *mapped_nents = dma_map_sg(dev, sg, *nents, direction);
> -               if (*mapped_nents == 0) {
> +               ret = dma_map_sg(dev, sg, *nents, direction);
> +               if (dma_mapping_error(dev, ret)) {
>                         *nents = 0;
> -                       dev_err(dev, "dma_map_sg() sg buffer failed\n");
> +                       dev_err(dev, "dma_map_sg() sg buffer failed %d\n", ret);
>                         return -ENOMEM;
>                 }
> +
> +               *mapped_nents = ret;
>         }
>
>         return 0;

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds