BUG: Seems un-initialed dst pointer received from algif_aead when outlen is zero

BUG: Seems un-initialed dst pointer received from algif_aead when outlen is zero

[Harsh Jain]

Hi,

For tag only AEAD decrypt operation(Zero length Payload). The dst sg
list pointer panic with general protection fault. I think it should be
NULL when output buffer is supposed to be empty.

Kcapi command to re-produce the issue

./kcapi -x 2   -c "gcm(aes)" -i 0d92aa861746b324f20ee6b7 -k
f4a6a5e5f2066f6dd9ec6fc5169c29043560ef595c9e81e76f42d29212cc581c -a ""
-t "5f24c68cbe6f32c29652442bf5d483ad" -q ""

Its decrypt operation. Expected result should be EBADMSG.


Regards
Harsh Jain

Re: BUG: Seems un-initialed dst pointer received from algif_aead when outlen is zero

[Stephan Müller]

Am Dienstag, 21. März 2017, 07:13:53 CET schrieb Harsh Jain:

Hi Harsh,

> Hi,
> 
> For tag only AEAD decrypt operation(Zero length Payload). The dst sg
> list pointer panic with general protection fault. I think it should be
> NULL when output buffer is supposed to be empty.
> 
> Kcapi command to re-produce the issue
> 
> ./kcapi -x 2   -c "gcm(aes)" -i 0d92aa861746b324f20ee6b7 -k
> f4a6a5e5f2066f6dd9ec6fc5169c29043560ef595c9e81e76f42d29212cc581c -a ""
> -t "5f24c68cbe6f32c29652442bf5d483ad" -q ""
> 
> Its decrypt operation. Expected result should be EBADMSG.

Executing this command on a 4.9 kernel, I get:

bin/kcapi -x 2   -c "gcm(aes)" -i 0d92aa861746b324f20ee6b7 -k 
f4a6a5e5f2066f6dd9ec6fc5169c29043560ef595c9e81e76f42d29212cc581c -a "" -t 
"5f24c68cbe6f32c29652442bf5d483ad" -q ""
EBADMSG

There is no GP or other error. Can you please provide some details about your 
system? I.e. which kernel version and what cipher implementation resolves to 
gcm(aes)?

Thanks

Ciao
Stephan

Re: BUG: Seems un-initialed dst pointer received from algif_aead when outlen is zero

[Harsh Jain]

On Tue, Mar 21, 2017 at 3:34 PM, Stephan Müller <smueller@chronox.de> wrote:
> Am Dienstag, 21. März 2017, 07:13:53 CET schrieb Harsh Jain:
>
> Hi Harsh,
>
>> Hi,
>>
>> For tag only AEAD decrypt operation(Zero length Payload). The dst sg
>> list pointer panic with general protection fault. I think it should be
>> NULL when output buffer is supposed to be empty.
>>
>> Kcapi command to re-produce the issue
>>
>> ./kcapi -x 2   -c "gcm(aes)" -i 0d92aa861746b324f20ee6b7 -k
>> f4a6a5e5f2066f6dd9ec6fc5169c29043560ef595c9e81e76f42d29212cc581c -a ""
>> -t "5f24c68cbe6f32c29652442bf5d483ad" -q ""
>>
>> Its decrypt operation. Expected result should be EBADMSG.
>
> Executing this command on a 4.9 kernel, I get:
>
> bin/kcapi -x 2   -c "gcm(aes)" -i 0d92aa861746b324f20ee6b7 -k
> f4a6a5e5f2066f6dd9ec6fc5169c29043560ef595c9e81e76f42d29212cc581c -a "" -t
> "5f24c68cbe6f32c29652442bf5d483ad" -q ""
> EBADMSG

Probably because s/w implementation is not trying to access dst sg
pointer because there's nothing to copy in destination buffer.  1
question If we don't have data to copy to destination buffer what
should dst pointer contains?

>
> There is no GP or other error. Can you please provide some details about your
> system? I.e. which kernel version and what cipher implementation resolves to
> gcm(aes)?

I tried with 4.10.13. It's with gcm(aes-chcr). changes which trigger
issue is not submitted to community yet.

>
> Thanks
>
> Ciao
> Stephan

Re: BUG: Seems un-initialed dst pointer received from algif_aead when outlen is zero

[Harsh Jain]

On Tue, Mar 21, 2017 at 4:29 PM, Harsh Jain <harshjain.prof@gmail.com> wrote:
> On Tue, Mar 21, 2017 at 3:34 PM, Stephan Müller <smueller@chronox.de> wrote:
>> Am Dienstag, 21. März 2017, 07:13:53 CET schrieb Harsh Jain:
>>
>> Hi Harsh,
>>
>>> Hi,
>>>
>>> For tag only AEAD decrypt operation(Zero length Payload). The dst sg
>>> list pointer panic with general protection fault. I think it should be
>>> NULL when output buffer is supposed to be empty.
>>>
>>> Kcapi command to re-produce the issue
>>>
>>> ./kcapi -x 2   -c "gcm(aes)" -i 0d92aa861746b324f20ee6b7 -k
>>> f4a6a5e5f2066f6dd9ec6fc5169c29043560ef595c9e81e76f42d29212cc581c -a ""
>>> -t "5f24c68cbe6f32c29652442bf5d483ad" -q ""
>>>
>>> Its decrypt operation. Expected result should be EBADMSG.
>>
>> Executing this command on a 4.9 kernel, I get:
>>
>> bin/kcapi -x 2   -c "gcm(aes)" -i 0d92aa861746b324f20ee6b7 -k
>> f4a6a5e5f2066f6dd9ec6fc5169c29043560ef595c9e81e76f42d29212cc581c -a "" -t
>> "5f24c68cbe6f32c29652442bf5d483ad" -q ""
>> EBADMSG
>
> Probably because s/w implementation is not trying to access dst sg
> pointer because there's nothing to copy in destination buffer.  1
> question If we don't have data to copy to destination buffer what
> should dst pointer contains? I think either NULL or null sg entry.
>
>>
>> There is no GP or other error. Can you please provide some details about your
>> system? I.e. which kernel version and what cipher implementation resolves to
>> gcm(aes)?
>
> I tried with 4.10.13. It's with gcm(aes-chcr). changes which trigger
> issue is not submitted to community yet.
    typo Its 4.10.0-rc3+
>
>>
>> Thanks
>>
>> Ciao
>> Stephan

Re: BUG: Seems un-initialed dst pointer received from algif_aead when outlen is zero

[Stephan Müller]

Am Dienstag, 21. März 2017, 11:59:54 CET schrieb Harsh Jain:

Hi Harsh,

> > Executing this command on a 4.9 kernel, I get:
> > 
> > bin/kcapi -x 2   -c "gcm(aes)" -i 0d92aa861746b324f20ee6b7 -k
> > f4a6a5e5f2066f6dd9ec6fc5169c29043560ef595c9e81e76f42d29212cc581c -a "" -t
> > "5f24c68cbe6f32c29652442bf5d483ad" -q ""
> > EBADMSG
> 
> Probably because s/w implementation is not trying to access dst sg
> pointer because there's nothing to copy in destination buffer.  1
> question If we don't have data to copy to destination buffer what
> should dst pointer contains?

The dst SGL should simply be discarded by implementations in the case you 
mention above.

The implementation receives the tag size and the supplied input buffer. If 
that input buffer length is equal to the tag length (i.e. no AAD and no 
ciphertext), why would the dst SGL be ever touched during decrytion?

Ciao
Stephan

Re: BUG: Seems un-initialed dst pointer received from algif_aead when outlen is zero

[Harsh Jain]

On Tue, Mar 21, 2017 at 5:13 PM, Stephan Müller <smueller@chronox.de> wrote:
> Am Dienstag, 21. März 2017, 11:59:54 CET schrieb Harsh Jain:
>
> Hi Harsh,
>
>> > Executing this command on a 4.9 kernel, I get:
>> >
>> > bin/kcapi -x 2   -c "gcm(aes)" -i 0d92aa861746b324f20ee6b7 -k
>> > f4a6a5e5f2066f6dd9ec6fc5169c29043560ef595c9e81e76f42d29212cc581c -a "" -t
>> > "5f24c68cbe6f32c29652442bf5d483ad" -q ""
>> > EBADMSG
>>
>> Probably because s/w implementation is not trying to access dst sg
>> pointer because there's nothing to copy in destination buffer.  1
>> question If we don't have data to copy to destination buffer what
>> should dst pointer contains?
>
> The dst SGL should simply be discarded by implementations in the case you
> mention above.
>
> The implementation receives the tag size and the supplied input buffer. If
> that input buffer length is equal to the tag length (i.e. no AAD and no
> ciphertext), why would the dst SGL be ever touched during decrytion?

Yes, Driver can figure out when to discard dst SGL but for that Driver
has to put checks before accessing dst SGL. Isn't better if AF_ALG
sends NULL for dst SGL.

>
> Ciao
> Stephan

Re: BUG: Seems un-initialed dst pointer received from algif_aead when outlen is zero

[Stephan Müller]

Am Dienstag, 21. März 2017, 14:23:31 CET schrieb Harsh Jain:

Hi Harsh,

> Yes, Driver can figure out when to discard dst SGL but for that Driver
> has to put checks before accessing dst SGL. Isn't better if AF_ALG
> sends NULL for dst SGL.

With the code in [1], the first longer patch is planned to be merged after the 
memory management changes are agreed upon. That patch contains:

+               /* chain the areq TX SGL holding the tag with RX SGL */
+               if (!last_rsgl) {
+                       /* no RX SGL present (e.g. only authentication) */
+                       sg_init_table(areq->first_rsgl.sgl.sg, 2);
+                       sg_chain(areq->first_rsgl.sgl.sg, 2, areq->tsgl);
+               } else {
+                       /* RX SGL present */
+                       struct af_alg_sgl *sgl_prev = &last_rsgl->sgl;
+
+                       sg_unmark_end(sgl_prev->sg + sgl_prev->npages - 1);
+                       sg_chain(sgl_prev->sg, sgl_prev->npages + 1, areq-
>tsgl);
+               }


This code snipped would exactly do what you want: the SGL is always 
initialized. Besides, the code will do an in-place cipher operation.

https://www.spinics.net/lists/linux-crypto/msg24343.html

Ciao
Stephan

Re: BUG: Seems un-initialed dst pointer received from algif_aead when outlen is zero

[Herbert Xu]

On Tue, Mar 21, 2017 at 04:00:04PM +0100, Stephan Müller wrote:
> Am Dienstag, 21. März 2017, 14:23:31 CET schrieb Harsh Jain:
> 
> Hi Harsh,
> 
> > Yes, Driver can figure out when to discard dst SGL but for that Driver
> > has to put checks before accessing dst SGL. Isn't better if AF_ALG
> > sends NULL for dst SGL.
> 
> With the code in [1], the first longer patch is planned to be merged after the 
> memory management changes are agreed upon. That patch contains:
> 
> +               /* chain the areq TX SGL holding the tag with RX SGL */
> +               if (!last_rsgl) {
> +                       /* no RX SGL present (e.g. only authentication) */
> +                       sg_init_table(areq->first_rsgl.sgl.sg, 2);
> +                       sg_chain(areq->first_rsgl.sgl.sg, 2, areq->tsgl);
> +               } else {
> +                       /* RX SGL present */
> +                       struct af_alg_sgl *sgl_prev = &last_rsgl->sgl;
> +
> +                       sg_unmark_end(sgl_prev->sg + sgl_prev->npages - 1);
> +                       sg_chain(sgl_prev->sg, sgl_prev->npages + 1, areq-
> >tsgl);
> +               }
> 
> 
> This code snipped would exactly do what you want: the SGL is always 
> initialized. Besides, the code will do an in-place cipher operation.
> 
> https://www.spinics.net/lists/linux-crypto/msg24343.html

Even if we fix this one user of the crypto API, new users could
still feed you bogus SG lists.  The API does not require the user
to specify a NULL SG list so please fix this in the driver.

We should also strength testmgr so that it provides something
bogus to catch buggy drivers.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt