diff mbox series

delta: decrypt zchunk header

Message ID 20230322114324.753-1-sirjme@gmail.com
State Accepted
Headers show
Series delta: decrypt zchunk header | expand

Commit Message

John Michael Edgware March 22, 2023, 11:43 a.m. UTC 
Signed-off-by: John Michael Edgware <sirjme@gmail.com>
---
 handlers/delta_handler.c | 38 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)

Comments

Stefano Babic March 26, 2023, 3:34 p.m. UTC | #1 
Ji John,

On 22.03.23 12:43, John Michael Edgware wrote:
> Signed-off-by: John Michael Edgware <sirjme@gmail.com>
> ---
>   handlers/delta_handler.c | 38 ++++++++++++++++++++++++++++++++++++--
>   1 file changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/handlers/delta_handler.c b/handlers/delta_handler.c
> index e848f15..e078157 100644
> --- a/handlers/delta_handler.c
> +++ b/handlers/delta_handler.c
> @@ -37,6 +37,7 @@
>   #include <pctl.h>
>   #include <pthread.h>
>   #include <fs_interface.h>
> +#include <sys/mman.h>
>   #include "delta_handler.h"
>   #include "multipart_parser.h"
>   #include "installer.h"
> @@ -840,7 +841,7 @@ static int install_delta(struct img_type *img,
>   {
>   	struct hnd_priv *priv;
>   	int ret = -1;
> -	int dst_fd = -1, in_fd = -1;
> +	int dst_fd = -1, in_fd = -1, mem_fd = -1;
>   	zckChunk *iter;
>   	zckCtx *zckSrc = NULL, *zckDst = NULL;
>   	char *FIFO = NULL;
> @@ -965,7 +966,37 @@ static int install_delta(struct img_type *img,
>   			zck_get_error(zckSrc));
>   		goto cleanup;
>   	}
> -	if (!zck_init_read(zckDst, img->fdin)) {
> +
> +	mem_fd = memfd_create("zchunk header", 0);
> +	if (mem_fd == -1) {
> +		ERROR("Cannot create memory file: %s", strerror(errno));
> +		goto cleanup;

See report sent by coverity when upur patch is applied. mem_fd is 
negative, and jumping to cleanup calls close() with a negative number. 
This issue needs to be solved before applying.

Best regards,
Stefano Babic


> +	}
> +
> +	ret = copyfile(img->fdin,
> +		&mem_fd,
> +		img->size,
> +		(unsigned long *)&img->offset,
> +		img->seek,
> +		0,
> +		img->compressed,
> +		&img->checksum,
> +		img->sha256,
> +		img->is_encrypted,
> +		img->ivt_ascii,
> +		NULL);
> +
> +	if (ret != 0) {
> +		ERROR("Error %d copying zchunk header, aborting.", ret);
> +		goto cleanup;
> +	}
> +
> +	if (lseek(mem_fd, 0, SEEK_SET) < 0) {
> +		ERROR("Seeking start of memory file");
> +		goto cleanup;
> +	}
> +
> +	if (!zck_init_read(zckDst, mem_fd)) {
>   		ERROR("Unable to read ZCK header from %s : %s",
>   			img->fname,
>   			zck_get_error(zckDst));
> @@ -1020,6 +1051,8 @@ static int install_delta(struct img_type *img,
>   	memset(priv_hnd->img.sha256, 0, SHA256_HASH_LENGTH);
>   	strlcpy(priv_hnd->img.type, priv->chainhandler, sizeof(priv_hnd->img.type));
>   	priv_hnd->img.fdin = pipes[PIPE_READ];
> +	/* zchunk files are not encrypted, CBC is not suitable for range download */
> +	priv_hnd->img.is_encrypted = false;
>   
>   	signal(SIGPIPE, SIG_IGN);
>   
> @@ -1064,6 +1097,7 @@ cleanup:
>   	if (zckDst) zck_free(&zckDst);
>   	close(dst_fd);
>   	close(in_fd);
> +	close(mem_fd);
>   	if (FIFO) {
>   		unlink(FIFO);
>   		free(FIFO);
John Michael Edgware March 26, 2023, 3:46 p.m. UTC | #2 
Hi Stefano,

I usually don't do this even though close(-1) will silently fail with EBADF 
without any adverse effects, but I took example from close(dst_fd) and 
close(in_fd) which also lack -1 check.

On Sunday, March 26, 2023 at 6:34:14 PM UTC+3 Stefano Babic wrote:

> Ji John,
>
> On 22.03.23 12:43, John Michael Edgware wrote:
> > Signed-off-by: John Michael Edgware <sir...@gmail.com>
> > ---
> > handlers/delta_handler.c | 38 ++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 36 insertions(+), 2 deletions(-)
> > 
> > diff --git a/handlers/delta_handler.c b/handlers/delta_handler.c
> > index e848f15..e078157 100644
> > --- a/handlers/delta_handler.c
> > +++ b/handlers/delta_handler.c
> > @@ -37,6 +37,7 @@
> > #include <pctl.h>
> > #include <pthread.h>
> > #include <fs_interface.h>
> > +#include <sys/mman.h>
> > #include "delta_handler.h"
> > #include "multipart_parser.h"
> > #include "installer.h"
> > @@ -840,7 +841,7 @@ static int install_delta(struct img_type *img,
> > {
> > struct hnd_priv *priv;
> > int ret = -1;
> > - int dst_fd = -1, in_fd = -1;
> > + int dst_fd = -1, in_fd = -1, mem_fd = -1;
> > zckChunk *iter;
> > zckCtx *zckSrc = NULL, *zckDst = NULL;
> > char *FIFO = NULL;
> > @@ -965,7 +966,37 @@ static int install_delta(struct img_type *img,
> > zck_get_error(zckSrc));
> > goto cleanup;
> > }
> > - if (!zck_init_read(zckDst, img->fdin)) {
> > +
> > + mem_fd = memfd_create("zchunk header", 0);
> > + if (mem_fd == -1) {
> > + ERROR("Cannot create memory file: %s", strerror(errno));
> > + goto cleanup;
>
> See report sent by coverity when upur patch is applied. mem_fd is 
> negative, and jumping to cleanup calls close() with a negative number. 
> This issue needs to be solved before applying.
>
> Best regards,
> Stefano Babic
>
>
> > + }
> > +
> > + ret = copyfile(img->fdin,
> > + &mem_fd,
> > + img->size,
> > + (unsigned long *)&img->offset,
> > + img->seek,
> > + 0,
> > + img->compressed,
> > + &img->checksum,
> > + img->sha256,
> > + img->is_encrypted,
> > + img->ivt_ascii,
> > + NULL);
> > +
> > + if (ret != 0) {
> > + ERROR("Error %d copying zchunk header, aborting.", ret);
> > + goto cleanup;
> > + }
> > +
> > + if (lseek(mem_fd, 0, SEEK_SET) < 0) {
> > + ERROR("Seeking start of memory file");
> > + goto cleanup;
> > + }
> > +
> > + if (!zck_init_read(zckDst, mem_fd)) {
> > ERROR("Unable to read ZCK header from %s : %s",
> > img->fname,
> > zck_get_error(zckDst));
> > @@ -1020,6 +1051,8 @@ static int install_delta(struct img_type *img,
> > memset(priv_hnd->img.sha256, 0, SHA256_HASH_LENGTH);
> > strlcpy(priv_hnd->img.type, priv->chainhandler, 
> sizeof(priv_hnd->img.type));
> > priv_hnd->img.fdin = pipes[PIPE_READ];
> > + /* zchunk files are not encrypted, CBC is not suitable for range 
> download */
> > + priv_hnd->img.is_encrypted = false;
> > 
> > signal(SIGPIPE, SIG_IGN);
> > 
> > @@ -1064,6 +1097,7 @@ cleanup:
> > if (zckDst) zck_free(&zckDst);
> > close(dst_fd);
> > close(in_fd);
> > + close(mem_fd);
> > if (FIFO) {
> > unlink(FIFO);
> > free(FIFO);
>
> -- 
> =====================================================================
> DENX Software Engineering GmbH, Managing Director: Erika Unter
> HRB 165235 Munich, Office: Kirchenstr.5, 82194 Groebenzell, Germany
> Phone: +49-8142-66989-53 <+49%208142%206698953> Fax: +49-8142-66989-80 
> <+49%208142%206698980> Email: sba...@denx.de
> =====================================================================
>
Stefano Babic March 26, 2023, 8:46 p.m. UTC | #3 
Hi John,

On 26.03.23 17:46, John Michael Edgware wrote:
> Hi Stefano,
> 
> I usually don't do this even though close(-1) will silently fail with 
> EBADF without any adverse effects, but I took example from close(dst_fd) 
> and close(in_fd) which also lack -1 check.

True - let's do in this way. I apply the patch, check for -1 can be done 
in a follow up patch. Sometimes in code there is the check, sometimes not.

Regards,
Stefano

> 
> On Sunday, March 26, 2023 at 6:34:14 PM UTC+3 Stefano Babic wrote:
> 
>     Ji John,
> 
>     On 22.03.23 12:43, John Michael Edgware wrote:
>      > Signed-off-by: John Michael Edgware <sir...@gmail.com>
>      > ---
>      > handlers/delta_handler.c | 38 ++++++++++++++++++++++++++++++++++++--
>      > 1 file changed, 36 insertions(+), 2 deletions(-)
>      >
>      > diff --git a/handlers/delta_handler.c b/handlers/delta_handler.c
>      > index e848f15..e078157 100644
>      > --- a/handlers/delta_handler.c
>      > +++ b/handlers/delta_handler.c
>      > @@ -37,6 +37,7 @@
>      > #include <pctl.h>
>      > #include <pthread.h>
>      > #include <fs_interface.h>
>      > +#include <sys/mman.h>
>      > #include "delta_handler.h"
>      > #include "multipart_parser.h"
>      > #include "installer.h"
>      > @@ -840,7 +841,7 @@ static int install_delta(struct img_type *img,
>      > {
>      > struct hnd_priv *priv;
>      > int ret = -1;
>      > - int dst_fd = -1, in_fd = -1;
>      > + int dst_fd = -1, in_fd = -1, mem_fd = -1;
>      > zckChunk *iter;
>      > zckCtx *zckSrc = NULL, *zckDst = NULL;
>      > char *FIFO = NULL;
>      > @@ -965,7 +966,37 @@ static int install_delta(struct img_type *img,
>      > zck_get_error(zckSrc));
>      > goto cleanup;
>      > }
>      > - if (!zck_init_read(zckDst, img->fdin)) {
>      > +
>      > + mem_fd = memfd_create("zchunk header", 0);
>      > + if (mem_fd == -1) {
>      > + ERROR("Cannot create memory file: %s", strerror(errno));
>      > + goto cleanup;
> 
>     See report sent by coverity when upur patch is applied. mem_fd is
>     negative, and jumping to cleanup calls close() with a negative number.
>     This issue needs to be solved before applying.
> 
>     Best regards,
>     Stefano Babic
> 
> 
>      > + }
>      > +
>      > + ret = copyfile(img->fdin,
>      > + &mem_fd,
>      > + img->size,
>      > + (unsigned long *)&img->offset,
>      > + img->seek,
>      > + 0,
>      > + img->compressed,
>      > + &img->checksum,
>      > + img->sha256,
>      > + img->is_encrypted,
>      > + img->ivt_ascii,
>      > + NULL);
>      > +
>      > + if (ret != 0) {
>      > + ERROR("Error %d copying zchunk header, aborting.", ret);
>      > + goto cleanup;
>      > + }
>      > +
>      > + if (lseek(mem_fd, 0, SEEK_SET) < 0) {
>      > + ERROR("Seeking start of memory file");
>      > + goto cleanup;
>      > + }
>      > +
>      > + if (!zck_init_read(zckDst, mem_fd)) {
>      > ERROR("Unable to read ZCK header from %s : %s",
>      > img->fname,
>      > zck_get_error(zckDst));
>      > @@ -1020,6 +1051,8 @@ static int install_delta(struct img_type *img,
>      > memset(priv_hnd->img.sha256, 0, SHA256_HASH_LENGTH);
>      > strlcpy(priv_hnd->img.type, priv->chainhandler,
>     sizeof(priv_hnd->img.type));
>      > priv_hnd->img.fdin = pipes[PIPE_READ];
>      > + /* zchunk files are not encrypted, CBC is not suitable for
>     range download */
>      > + priv_hnd->img.is_encrypted = false;
>      >
>      > signal(SIGPIPE, SIG_IGN);
>      >
>      > @@ -1064,6 +1097,7 @@ cleanup:
>      > if (zckDst) zck_free(&zckDst);
>      > close(dst_fd);
>      > close(in_fd);
>      > + close(mem_fd);
>      > if (FIFO) {
>      > unlink(FIFO);
>      > free(FIFO);
> 
>     -- 
>     =====================================================================
>     DENX Software Engineering GmbH, Managing Director: Erika Unter
>     HRB 165235 Munich, Office: Kirchenstr.5, 82194 Groebenzell, Germany
>     Phone: +49-8142-66989-53 <tel:+49%208142%206698953> Fax:
>     +49-8142-66989-80 <tel:+49%208142%206698980> Email: sba...@denx.de
>     =====================================================================
> 
> -- 
> You received this message because you are subscribed to the Google 
> Groups "swupdate" group.
> To unsubscribe from this group and stop receiving emails from it, send 
> an email to swupdate+unsubscribe@googlegroups.com 
> <mailto:swupdate+unsubscribe@googlegroups.com>.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/swupdate/80cb66f5-45ff-44da-8af0-008bb3eed905n%40googlegroups.com <https://groups.google.com/d/msgid/swupdate/80cb66f5-45ff-44da-8af0-008bb3eed905n%40googlegroups.com?utm_medium=email&utm_source=footer>.
John Michael Edgware March 27, 2023, 5:55 a.m. UTC | #4 
Great, thank you!

On Sunday, March 26, 2023 at 11:46:49 PM UTC+3 Stefano Babic wrote:

> Hi John,
>
> On 26.03.23 17:46, John Michael Edgware wrote:
> > Hi Stefano,
> > 
> > I usually don't do this even though close(-1) will silently fail with 
> > EBADF without any adverse effects, but I took example from close(dst_fd) 
> > and close(in_fd) which also lack -1 check.
>
> True - let's do in this way. I apply the patch, check for -1 can be done 
> in a follow up patch. Sometimes in code there is the check, sometimes not.
>
> Regards,
> Stefano
>
> > 
> > On Sunday, March 26, 2023 at 6:34:14 PM UTC+3 Stefano Babic wrote:
> > 
> > Ji John,
> > 
> > On 22.03.23 12:43, John Michael Edgware wrote:
> > > Signed-off-by: John Michael Edgware <sir...@gmail.com>
> > > ---
> > > handlers/delta_handler.c | 38 ++++++++++++++++++++++++++++++++++++--
> > > 1 file changed, 36 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/handlers/delta_handler.c b/handlers/delta_handler.c
> > > index e848f15..e078157 100644
> > > --- a/handlers/delta_handler.c
> > > +++ b/handlers/delta_handler.c
> > > @@ -37,6 +37,7 @@
> > > #include <pctl.h>
> > > #include <pthread.h>
> > > #include <fs_interface.h>
> > > +#include <sys/mman.h>
> > > #include "delta_handler.h"
> > > #include "multipart_parser.h"
> > > #include "installer.h"
> > > @@ -840,7 +841,7 @@ static int install_delta(struct img_type *img,
> > > {
> > > struct hnd_priv *priv;
> > > int ret = -1;
> > > - int dst_fd = -1, in_fd = -1;
> > > + int dst_fd = -1, in_fd = -1, mem_fd = -1;
> > > zckChunk *iter;
> > > zckCtx *zckSrc = NULL, *zckDst = NULL;
> > > char *FIFO = NULL;
> > > @@ -965,7 +966,37 @@ static int install_delta(struct img_type *img,
> > > zck_get_error(zckSrc));
> > > goto cleanup;
> > > }
> > > - if (!zck_init_read(zckDst, img->fdin)) {
> > > +
> > > + mem_fd = memfd_create("zchunk header", 0);
> > > + if (mem_fd == -1) {
> > > + ERROR("Cannot create memory file: %s", strerror(errno));
> > > + goto cleanup;
> > 
> > See report sent by coverity when upur patch is applied. mem_fd is
> > negative, and jumping to cleanup calls close() with a negative number.
> > This issue needs to be solved before applying.
> > 
> > Best regards,
> > Stefano Babic
> > 
> > 
> > > + }
> > > +
> > > + ret = copyfile(img->fdin,
> > > + &mem_fd,
> > > + img->size,
> > > + (unsigned long *)&img->offset,
> > > + img->seek,
> > > + 0,
> > > + img->compressed,
> > > + &img->checksum,
> > > + img->sha256,
> > > + img->is_encrypted,
> > > + img->ivt_ascii,
> > > + NULL);
> > > +
> > > + if (ret != 0) {
> > > + ERROR("Error %d copying zchunk header, aborting.", ret);
> > > + goto cleanup;
> > > + }
> > > +
> > > + if (lseek(mem_fd, 0, SEEK_SET) < 0) {
> > > + ERROR("Seeking start of memory file");
> > > + goto cleanup;
> > > + }
> > > +
> > > + if (!zck_init_read(zckDst, mem_fd)) {
> > > ERROR("Unable to read ZCK header from %s : %s",
> > > img->fname,
> > > zck_get_error(zckDst));
> > > @@ -1020,6 +1051,8 @@ static int install_delta(struct img_type *img,
> > > memset(priv_hnd->img.sha256, 0, SHA256_HASH_LENGTH);
> > > strlcpy(priv_hnd->img.type, priv->chainhandler,
> > sizeof(priv_hnd->img.type));
> > > priv_hnd->img.fdin = pipes[PIPE_READ];
> > > + /* zchunk files are not encrypted, CBC is not suitable for
> > range download */
> > > + priv_hnd->img.is_encrypted = false;
> > >
> > > signal(SIGPIPE, SIG_IGN);
> > >
> > > @@ -1064,6 +1097,7 @@ cleanup:
> > > if (zckDst) zck_free(&zckDst);
> > > close(dst_fd);
> > > close(in_fd);
> > > + close(mem_fd);
> > > if (FIFO) {
> > > unlink(FIFO);
> > > free(FIFO);
> > 
> > -- 
> > =====================================================================
> > DENX Software Engineering GmbH, Managing Director: Erika Unter
> > HRB 165235 Munich, Office: Kirchenstr.5, 82194 Groebenzell, Germany
> > Phone: +49-8142-66989-53 <+49%208142%206698953> 
> <tel:+49%208142%206698953> Fax:
> > +49-8142-66989-80 <+49%208142%206698980> <tel:+49%208142%206698980> 
> Email: sba...@denx.de
> > =====================================================================
> > 
> > -- 
> > You received this message because you are subscribed to the Google 
> > Groups "swupdate" group.
> > To unsubscribe from this group and stop receiving emails from it, send 
> > an email to swupdate+u...@googlegroups.com 
> > <mailto:swupdate+u...@googlegroups.com>.
> > To view this discussion on the web visit 
> > 
> https://groups.google.com/d/msgid/swupdate/80cb66f5-45ff-44da-8af0-008bb3eed905n%40googlegroups.com 
> <
> https://groups.google.com/d/msgid/swupdate/80cb66f5-45ff-44da-8af0-008bb3eed905n%40googlegroups.com?utm_medium=email&utm_source=footer
> >.
>
> -- 
> =====================================================================
> DENX Software Engineering GmbH, Managing Director: Erika Unter
> HRB 165235 Munich, Office: Kirchenstr.5, 82194 Groebenzell, Germany
> Phone: +49-8142-66989-53 <+49%208142%206698953> Fax: +49-8142-66989-80 
> <+49%208142%206698980> Email: sba...@denx.de
> =====================================================================
>
diff mbox series

Patch

diff --git a/handlers/delta_handler.c b/handlers/delta_handler.c
index e848f15..e078157 100644
--- a/handlers/delta_handler.c
+++ b/handlers/delta_handler.c
@@ -37,6 +37,7 @@ 
 #include <pctl.h>
 #include <pthread.h>
 #include <fs_interface.h>
+#include <sys/mman.h>
 #include "delta_handler.h"
 #include "multipart_parser.h"
 #include "installer.h"
@@ -840,7 +841,7 @@  static int install_delta(struct img_type *img,
 {
 	struct hnd_priv *priv;
 	int ret = -1;
-	int dst_fd = -1, in_fd = -1;
+	int dst_fd = -1, in_fd = -1, mem_fd = -1;
 	zckChunk *iter;
 	zckCtx *zckSrc = NULL, *zckDst = NULL;
 	char *FIFO = NULL;
@@ -965,7 +966,37 @@  static int install_delta(struct img_type *img,
 			zck_get_error(zckSrc));
 		goto cleanup;
 	}
-	if (!zck_init_read(zckDst, img->fdin)) {
+
+	mem_fd = memfd_create("zchunk header", 0);
+	if (mem_fd == -1) {
+		ERROR("Cannot create memory file: %s", strerror(errno));
+		goto cleanup;
+	}
+
+	ret = copyfile(img->fdin,
+		&mem_fd,
+		img->size,
+		(unsigned long *)&img->offset,
+		img->seek,
+		0,
+		img->compressed,
+		&img->checksum,
+		img->sha256,
+		img->is_encrypted,
+		img->ivt_ascii,
+		NULL);
+
+	if (ret != 0) {
+		ERROR("Error %d copying zchunk header, aborting.", ret);
+		goto cleanup;
+	}
+
+	if (lseek(mem_fd, 0, SEEK_SET) < 0) {
+		ERROR("Seeking start of memory file");
+		goto cleanup;
+	}
+
+	if (!zck_init_read(zckDst, mem_fd)) {
 		ERROR("Unable to read ZCK header from %s : %s",
 			img->fname,
 			zck_get_error(zckDst));
@@ -1020,6 +1051,8 @@  static int install_delta(struct img_type *img,
 	memset(priv_hnd->img.sha256, 0, SHA256_HASH_LENGTH);
 	strlcpy(priv_hnd->img.type, priv->chainhandler, sizeof(priv_hnd->img.type));
 	priv_hnd->img.fdin = pipes[PIPE_READ];
+	/* zchunk files are not encrypted, CBC is not suitable for range download */
+	priv_hnd->img.is_encrypted = false;
 
 	signal(SIGPIPE, SIG_IGN);
 
@@ -1064,6 +1097,7 @@  cleanup:
 	if (zckDst) zck_free(&zckDst);
 	close(dst_fd);
 	close(in_fd);
+	close(mem_fd);
 	if (FIFO) {
 		unlink(FIFO);
 		free(FIFO);