Mailing List Archive: [PATCH 1/8] g10/decrypt-data: use fill

[PATCH 1/8] g10/decrypt-data: use fill_buffer in more places

Oct 27, 2018, 2:05 PM

Post #1 of 6 (530 views)

* g10/decrypt-data.c (mdc_decode_filter, decode_filter): Use
fill_buffer.
--

Signed-off-by: Jussi Kivilinna <jussi.kivilinna@iki.fi>
---
0 files changed

diff --git a/g10/decrypt-data.c b/g10/decrypt-data.c
index 3951fa794..d1d72a30f 100644
--- a/g10/decrypt-data.c
+++ b/g10/decrypt-data.c
@@ -873,7 +873,6 @@ mdc_decode_filter (void *opaque, int control, IOBUF a,
decode_filter_ctx_t dfx = opaque;
size_t n, size = *ret_len;
int rc = 0;
- int c;

/* Note: We need to distinguish between a partial and a fixed length
packet. The first is the usual case as created by GPG. However
@@ -894,25 +893,7 @@ mdc_decode_filter (void *opaque, int control, IOBUF a,
log_assert (size > 44); /* Our code requires at least this size. */

/* Get at least 22 bytes and put it ahead in the buffer. */
- if (dfx->partial)
- {
- for (n=22; n < 44; n++)
- {
- if ( (c = iobuf_get(a)) == -1 )
- break;
- buf[n] = c;
- }
- }
- else
- {
- for (n=22; n < 44 && dfx->length; n++, dfx->length--)
- {
- c = iobuf_get (a);
- if (c == -1)
- break; /* Premature EOF. */
- buf[n] = c;
- }
- }
+ n = fill_buffer (dfx, a, buf, 44, 22);
if (n == 44)
{
/* We have enough stuff - flush the holdback buffer. */
@@ -923,37 +904,11 @@ mdc_decode_filter (void *opaque, int control, IOBUF a,
}
else
{
-
memcpy (buf, dfx->holdback, 22);
}
+
/* Fill up the buffer. */
- if (dfx->partial)
- {
- for (; n < size; n++ )
- {
- if ( (c = iobuf_get(a)) == -1 )
- {
- dfx->eof_seen = 1; /* Normal EOF. */
- break;
- }
- buf[n] = c;
- }
- }
- else
- {
- for (; n < size && dfx->length; n++, dfx->length--)
- {
- c = iobuf_get(a);
- if (c == -1)
- {
- dfx->eof_seen = 3; /* Premature EOF. */
- break;
- }
- buf[n] = c;
- }
- if (!dfx->length)
- dfx->eof_seen = 1; /* Normal EOF. */
- }
+ n = fill_buffer (dfx, a, buf, size, n);

/* Move the trailing 22 bytes back to the holdback buffer. We
have at least 44 bytes thus a memmove is not needed. */
@@ -1008,7 +963,7 @@ decode_filter( void *opaque, int control, IOBUF a, byte *buf, size_t *ret_len)
decode_filter_ctx_t fc = opaque;
size_t size = *ret_len;
size_t n;
- int c, rc = 0;
+ int rc = 0;

if ( control == IOBUFCTRL_UNDERFLOW && fc->eof_seen )
@@ -1020,34 +975,7 @@ decode_filter( void *opaque, int control, IOBUF a, byte *buf, size_t *ret_len)
{
log_assert (a);

- if (fc->partial)
- {
- for (n=0; n < size; n++ )
- {
- c = iobuf_get(a);
- if (c == -1)
- {
- fc->eof_seen = 1; /* Normal EOF. */
- break;
- }
- buf[n] = c;
- }
- }
- else
- {
- for (n=0; n < size && fc->length; n++, fc->length--)
- {
- c = iobuf_get(a);
- if (c == -1)
- {
- fc->eof_seen = 3; /* Premature EOF. */
- break;
- }
- buf[n] = c;
- }
- if (!fc->length)
- fc->eof_seen = 1; /* Normal EOF. */
- }
+ n = fill_buffer (fc, a, buf, size, 0);
if (n)
{
if (fc->cipher_hd)

_______________________________________________
Gnupg-devel mailing list
Gnupg-devel@gnupg.org
http://lists.gnupg.org/mailman/listinfo/gnupg-devel

Re: [PATCH 1/8] g10/decrypt-data: use fill_buffer in more places [ In reply to ]

wk at gnupg

Oct 29, 2018, 6:45 AM

Post #2 of 6 (528 views)

Permalink

Hi!

All patches look fine to me. After applying we should however run
extensive tests against another implementation to see whether we have
any breakage. With the current code this has been with the help of
the www.rnpgp.com folks.

Salam-Shalom,

Werner

--
Die Gedanken sind frei. Ausnahmen regelt ein Bundesgesetz.

Re: [PATCH 1/8] g10/decrypt-data: use fill_buffer in more places [ In reply to ]

jussi.kivilinna at iki

Nov 7, 2018, 9:38 AM

Post #3 of 6 (524 views)

Permalink

Hello,

On 29.10.2018 15.45, Werner Koch wrote:
> Hi!
>
> All patches look fine to me. After applying we should however run
> extensive tests against another implementation to see whether we have
> any breakage. With the current code this has been with the help of
> the www.rnpgp.com folks.

Should I push this patch set to new branch in gnupg repo?

I've also looked at disabling extra hash contexts when decrypting
non-signed files. Could those contexts be disabled when any AEAD or
MDC encrypted packet is seen? Such patch would look something this:

diff --git a/g10/mainproc.c b/g10/mainproc.c
index 5b7bc9555..4309b52ac 100644
--- a/g10/mainproc.c
+++ b/g10/mainproc.c
@@ -91,6 +91,8 @@ struct mainproc_context
has been seen. */
unsigned int data:1; /* Any data packet seen */
unsigned int uncompress_failed:1;
+ unsigned int seen_encrypted_mdc:1; /* Any PKT_ENCRYPTED_MDC packet seen */
+ unsigned int seen_encrypted_aead:1; /* Any PKT_ENCRYPTED_AEAD packet seen */
} any;
};

@@ -536,6 +538,9 @@ proc_encrypted (CTX c, PACKET *pkt)
int result = 0;
int early_plaintext = literals_seen;

+ c->any.seen_encrypted_mdc |= (pkt->pkttype == PKT_ENCRYPTED_MDC);
+ c->any.seen_encrypted_aead |= (pkt->pkttype == PKT_ENCRYPTED_AEAD);
+
if (early_plaintext)
{
log_info (_("WARNING: multiple plaintexts seen\n"));

@@ -874,7 +878,8 @@ proc_plaintext( CTX c, PACKET *pkt )
}
}

- if (!any && !opt.skip_verify)
+ if (!any && !opt.skip_verify && !c->any.seen_encrypted_mdc &&
+ !c->any.seen_encrypted_aead)
{
/* This is for the old GPG LITERAL+SIG case. It's not legal
according to 2440, so hopefully it won't come up that often.

-Jussi

_______________________________________________
Gnupg-devel mailing list
Gnupg-devel@gnupg.org
http://lists.gnupg.org/mailman/listinfo/gnupg-devel

Re: [PATCH 1/8] g10/decrypt-data: use fill_buffer in more places [ In reply to ]

wk at gnupg

Nov 7, 2018, 11:49 AM

Post #4 of 6 (524 views)

Permalink

On Wed, 7 Nov 2018 18:38, jussi.kivilinna@iki.fi said:

> Should I push this patch set to new branch in gnupg repo?

Just go ahead and push it directly to master.

> I've also looked at disabling extra hash contexts when decrypting
> non-signed files. Could those contexts be disabled when any AEAD or
> MDC encrypted packet is seen? Such patch would look something this:

While looking at your patches I was reminded to check whether we have
some useless hash context running.

> + unsigned int seen_encrypted_mdc:1; /* Any PKT_ENCRYPTED_MDC packet seen */
> + unsigned int seen_encrypted_aead:1; /* Any PKT_ENCRYPTED_AEAD packet seen */

There is either one MDC packet or one AEAD packet.

> - if (!any && !opt.skip_verify)
> + if (!any && !opt.skip_verify && !c->any.seen_encrypted_mdc &&
> + !c->any.seen_encrypted_aead)
> {
> /* This is for the old GPG LITERAL+SIG case. It's not legal
> according to 2440, so hopefully it won't come up that often.

For sure this is not possible with AEAD. With MDC it is unlikely but I
think we should not touch that case given that the goal is to fade out
the use of MDC.

Shalom-Salam,

Werner

--
Die Gedanken sind frei. Ausnahmen regelt ein Bundesgesetz.

Re: [PATCH 1/8] g10/decrypt-data: use fill_buffer in more places [ In reply to ]

jussi.kivilinna at iki

Nov 8, 2018, 10:38 AM

Post #5 of 6 (521 views)

Permalink

On 7.11.2018 21.49, Werner Koch wrote:
> On Wed, 7 Nov 2018 18:38, jussi.kivilinna@iki.fi said:
>
>> Should I push this patch set to new branch in gnupg repo?
>
> Just go ahead and push it directly to master.
>
>> I've also looked at disabling extra hash contexts when decrypting
>> non-signed files. Could those contexts be disabled when any AEAD or
>> MDC encrypted packet is seen? Such patch would look something this:
>
> While looking at your patches I was reminded to check whether we have
> some useless hash context running.
>
>> + unsigned int seen_encrypted_mdc:1; /* Any PKT_ENCRYPTED_MDC packet seen */
>> + unsigned int seen_encrypted_aead:1; /* Any PKT_ENCRYPTED_AEAD packet seen */
>
> There is either one MDC packet or one AEAD packet.
>
>> - if (!any && !opt.skip_verify)
>> + if (!any && !opt.skip_verify && !c->any.seen_encrypted_mdc &&
>> + !c->any.seen_encrypted_aead)
>> {
>> /* This is for the old GPG LITERAL+SIG case. It's not legal
>> according to 2440, so hopefully it won't come up that often.
>
> For sure this is not possible with AEAD. With MDC it is unlikely but I
> think we should not touch that case given that the goal is to fade out
> the use of MDC.
>

Ok, I'll make patch AEAD only. For CFB/MDC, user can of course use
--skip-verify if they know that input does not have signature and want
to get highest performance.

Here's results that I've seen with patch/--skip-verify for different types
of input on my machine (2GiB input file from ramfs):

decrypting MDC encrypted, signed (AES128+SHA1(mdc)+SHA512(sign)):
user 5.2s, 364 MB/s
decrypting MDC encrypted, not signed (AES128+2xSHA1(mdc+extra)+RMD160(extra)):
user 9.6s, 206 MB/s
decrypting MDC encrypted, not signed --skip-verify (AES128+SHA1(mdc)):
user 3.0s, 575 MB/s

decrypting MDC symmetric encrypted, not signed (AES128+SHA1(mdc+extra)+RMD160(extra)):
user 9.7s, 205 MB/s
decrypting MDC symmetric encrypted, not signed --skip-verify (AES128+SHA1(mdc)):
user 3.1s, 556 MB/s

decrypting AEAD encrypted, signed (AES128_OCB+SHA512(sign)):
user 4.7s, 387 MB/s
decrypting AEAD encrypted, not signed (AES128_OCB+SHA1(extra)+RMD160(extra)):
user 7.6s, 258 MB/s
decrypting AEAD encrypted, not signed --skip-verify or patched (AES128_OCB):
user 0.95s, 1.2 GB/s

decrypting AEAD symmetric encrypted, not signed (AES128_OCB+SHA1(extra)+RMD160(extra)):
user 7.6s, 256 MB/s
decrypting AEAD symmetric encrypted, not signed --skip-verify or patched (AES128_OCB):
user 1.1s, 1.1 GB/s

I also noticed that --skip-verify does not affect decryption speed of
signed input. Selected digest algorithm gets enabled regardless of
--skip-verify in proc_plaintext(). Should this be fixed?

-Jussi

Re: [PATCH 1/8] g10/decrypt-data: use fill_buffer in more places [ In reply to ]

wk at gnupg

Nov 13, 2018, 7:33 AM

Post #6 of 6 (514 views)

Permalink

On Thu, 8 Nov 2018 19:38, jussi.kivilinna@iki.fi said:

> Ok, I'll make patch AEAD only. For CFB/MDC, user can of course use
> --skip-verify if they know that input does not have signature and want
> to get highest performance.

We should add this to the FAQ under a new section how to speed up
operations.

> decrypting MDC encrypted, not signed (AES128+2xSHA1(mdc+extra)+RMD160(extra)):
> user 9.6s, 206 MB/s
> decrypting MDC encrypted, not signed --skip-verify (AES128+SHA1(mdc)):
> user 3.0s, 575 MB/s

The RMD160 is really really slow.

> decrypting AEAD encrypted, not signed (AES128_OCB+SHA1(extra)+RMD160(extra)):
> user 7.6s, 258 MB/s
> decrypting AEAD encrypted, not signed --skip-verify or patched (AES128_OCB):
> user 0.95s, 1.2 GB/s

Yeah, that is a speedup.

> I also noticed that --skip-verify does not affect decryption speed of
> signed input. Selected digest algorithm gets enabled regardless of
> --skip-verify in proc_plaintext(). Should this be fixed?

Yes, please. Performance was not an issue back in April 98 when I
implemented --skip-verify.

Shalom-Salam,

Werner

--
Die Gedanken sind frei. Ausnahmen regelt ein Bundesgesetz.

Mailing List Archive

Mailing List Archive

Attached Files: