Hi, I agree. Thanks a lot Olivier. Also thanks for the other 2 FIPS patches :-).
Regards, Fan > -----Original Message----- > From: Olivier Matz <olivier.m...@6wind.com> > Sent: Thursday, October 8, 2020 3:20 PM > To: Zhang, Roy Fan <roy.fan.zh...@intel.com> > Cc: dev@dpdk.org; Kovacevic, Marko <marko.kovace...@intel.com>; Akhil > Goyal <akhil.go...@nxp.com>; Kusztal, ArkadiuszX > <arkadiuszx.kusz...@intel.com>; sta...@dpdk.org; Anoob Joseph > <ano...@marvell.com> > Subject: Re: [PATCH 2/3] examples/fips_validation: ignore \r in input files > > Hi Fan, > > So if we cannot know which version removed the \r, I suggest to just > drop this patch. I thought it was a bug in the parser, but if it does > not happen with files matching the supported CAVS version, there is > nothing to fix. > > What do you think? > > Thanks, > Olivier > > > On Thu, Oct 08, 2020 at 12:41:11PM +0000, Zhang, Roy Fan wrote: > > Hi Olivier, > > > > Unfortunately I wanted to find the same document since forever. NIST > > did not provide this on their website. What I am sure is for CAVS 21.0 > > both the test vectors Intel used for testing and the ones provided by > > our customer for debugging did not have \r in the files. In 2018 we > > could find some sample request and response files from NIST website > > but I just checked and they are gone. > > > > Regards, > > Fan > > > > > -----Original Message----- > > > From: Olivier Matz <olivier.m...@6wind.com> > > > Sent: Thursday, October 8, 2020 12:32 PM > > > To: Zhang, Roy Fan <roy.fan.zh...@intel.com> > > > Cc: dev@dpdk.org; Kovacevic, Marko <marko.kovace...@intel.com>; > Akhil > > > Goyal <akhil.go...@nxp.com>; Kusztal, ArkadiuszX > > > <arkadiuszx.kusz...@intel.com>; sta...@dpdk.org; Anoob Joseph > > > <ano...@marvell.com> > > > Subject: Re: [PATCH 2/3] examples/fips_validation: ignore \r in input > > > files > > > > > > Hi Fan, > > > > > > Thank you for the clarification. One more question: do you know where I > > > can find a description of the different FIPS CAVS versions? I would like > > > to know from what version the \r has been removed. > > > > > > Thanks, > > > Olivier > > > > > > On Thu, Oct 08, 2020 at 10:24:48AM +0000, Zhang, Roy Fan wrote: > > > > Hi Olivier, > > > > > > > > Sorry I didn't state myself clear in the first place. > > > > > > > > My intention is '\r' check, or any future CAVS version specific change > > > > to > the > > > > application should be wrapped into a branch that is checked with > parsed > > > > version number. With this way the original application's behavior should > > > > remain the same. > > > > > > > > The reason for that is we are having an issue right now that the > validation > > > > team is struggling with the limited test vectors and inconsistency > formatting > > > > between different FIPS CAVS versions. For example we still have FIPS > TDES > > > test > > > > failing today due to the different test file versions. > > > > https://bugs.dpdk.org/show_bug.cgi?id=512 > > > > > > > > The solution is certainly far from pretty but should help to share the > > > > maintenance effort amongst the contributors. > > > > > > > > The "FIPS_DEF_VERSION" can be removed of course. > > > > > > > > Regards, > > > > Fan > > > > > > > > > -----Original Message----- > > > > > From: Olivier Matz <olivier.m...@6wind.com> > > > > > Sent: Thursday, October 8, 2020 10:22 AM > > > > > To: Zhang, Roy Fan <roy.fan.zh...@intel.com> > > > > > Cc: dev@dpdk.org; Kovacevic, Marko <marko.kovace...@intel.com>; > > > Akhil > > > > > Goyal <akhil.go...@nxp.com>; Kusztal, ArkadiuszX > > > > > <arkadiuszx.kusz...@intel.com>; sta...@dpdk.org; Anoob Joseph > > > > > <ano...@marvell.com> > > > > > Subject: Re: [PATCH 2/3] examples/fips_validation: ignore \r in input > files > > > > > > > > > > Hi, > > > > > > > > > > On Thu, Oct 08, 2020 at 08:50:25AM +0000, Zhang, Roy Fan wrote: > > > > > > Hi Olivier, > > > > > > > > > > > > Anood and us had the similar discussion. > > > > > > > > > > > > Can we change the sample application to parse version data instead, > > > > > > and for the version specific code changes we will wrap them by a > > > > > > branch to compare the parsed version and the expected version? > > > > > > (we probably should have done that long time ago). > > > > > > > > > > > > I drafted a code change to parse the version data, see if you think > > > > > > it > > > > > > is ok? > > > > > > > > > > Thank you for your feedback. > > > > > > > > > > The code that gets the version looks good to me (I just have a > > > > > small comment, see below). However I'm not sure what to do with it. > > > > > > > > > > Do you mean we should return an error if the version is incorrect? Or > > > > > should we only skip '\r' for old versions? FIPS_DEF_VERSION is not > used > > > > > in your patch. In that case, I think it is a bit overkill. Do you > > > > > think > > > > > it is a problem to always drop '\r'? > > > > > > > > > > If you think we should not support files containing '\r', I'm fine > > > > > with it, I can drop this particular patch. > > > > > > > > > > > > > > > > > > > > > > diff --git a/examples/fips_validation/fips_validation.c > > > > > b/examples/fips_validation/fips_validation.c > > > > > > index 9bdf257b8..9b6518c92 100644 > > > > > > --- a/examples/fips_validation/fips_validation.c > > > > > > +++ b/examples/fips_validation/fips_validation.c > > > > > > @@ -98,7 +98,7 @@ fips_test_parse_header(void) > > > > > > uint32_t i; > > > > > > char *tmp; > > > > > > int ret; > > > > > > - int algo_parsed = 0; > > > > > > + int algo_parsed = 0, version_parsed = 0; > > > > > > time_t t = time(NULL); > > > > > > struct tm *tm_now = localtime(&t); > > > > > > > > > > > > @@ -107,6 +107,27 @@ fips_test_parse_header(void) > > > > > > return ret; > > > > > > > > > > > > for (i = 0; i < info.nb_vec_lines; i++) { > > > > > > + /* parse the version info */ > > > > > > + tmp = strstr(info.vec[i], "CAVS "); > > > > > > + if (tmp != NULL) { > > > > > > + if (version_parsed != 0) { > > > > > > + RTE_LOG(ERR, USER1, > > > > > > + "Multiple version data\n"); > > > > > > + return -1; > > > > > > + } > > > > > > + > > > > > > + tmp = tmp + sizeof("CAVS "); > > > > > > > > > > I think it should be strlen(), because sizeof() will contain > > > > > the '\0'. Or it could be sizeof() - 1. > > > > > > > > > > > + > > > > > > + if (strlen(tmp) >= MAX_VER_STRING_SIZE) { > > > > > > + RTE_LOG(ERR, USER1, "Version (%s) > too > > > > > long\n", > > > > > > + tmp); > > > > > > + return -1; > > > > > > + } > > > > > > + > > > > > > + strlcpy(info.version, tmp, > MAX_VER_STRING_SIZE); > > > > > > + version_parsed = 1; > > > > > > + } > > > > > > + > > > > > > if (!algo_parsed) { > > > > > > if (strstr(info.vec[i], "AESVS")) { > > > > > > algo_parsed = 1; > > > > > > diff --git a/examples/fips_validation/fips_validation.h > > > > > b/examples/fips_validation/fips_validation.h > > > > > > index 75fa555fa..b8c60c55f 100644 > > > > > > --- a/examples/fips_validation/fips_validation.h > > > > > > +++ b/examples/fips_validation/fips_validation.h > > > > > > @@ -15,6 +15,9 @@ > > > > > > #define MAX_BUF_SIZE 2048 > > > > > > #define MAX_STRING_SIZE 64 > > > > > > #define MAX_DIGEST_SIZE 64 > > > > > > +#define MAX_VER_STRING_SIZE 8 > > > > > > + > > > > > > +#define FIPS_DEF_VERSION "21.0" > > > > > > > > > > > > #define POSITIVE_TEST 0 > > > > > > #define NEGATIVE_TEST -1 > > > > > > @@ -155,6 +158,7 @@ struct sha_interim_data { > > > > > > }; > > > > > > > > > > > > struct fips_test_interim_info { > > > > > > + char version[MAX_VER_STRING_SIZE]; > > > > > > FILE *fp_rd; > > > > > > FILE *fp_wr; > > > > > > enum file_types file_type; > > > > > > > > > > > > > > > > > > Regards, > > > > > > Fan > > > > > > > > > > > > > -----Original Message----- > > > > > > > From: Olivier Matz <olivier.m...@6wind.com> > > > > > > > Sent: Tuesday, October 6, 2020 11:09 AM > > > > > > > To: Zhang, Roy Fan <roy.fan.zh...@intel.com> > > > > > > > Cc: dev@dpdk.org; Kovacevic, Marko > <marko.kovace...@intel.com>; > > > > > Akhil > > > > > > > Goyal <akhil.go...@nxp.com>; Kusztal, ArkadiuszX > > > > > > > <arkadiuszx.kusz...@intel.com>; sta...@dpdk.org; Anoob Joseph > > > > > > > <ano...@marvell.com> > > > > > > > Subject: Re: [PATCH 2/3] examples/fips_validation: ignore \r in > input > > > files > > > > > > > > > > > > > > Hi Fan, > > > > > > > > > > > > > > On Tue, Oct 06, 2020 at 08:47:10AM +0000, Zhang, Roy Fan wrote: > > > > > > > > Hi Olivier, > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > > > > From: Olivier Matz <olivier.m...@6wind.com> > > > > > > > > > Sent: Tuesday, October 6, 2020 8:42 AM > > > > > > > > > To: dev@dpdk.org > > > > > > > > > Cc: Kovacevic, Marko <marko.kovace...@intel.com>; Akhil > Goyal > > > > > > > > > <akhil.go...@nxp.com>; Zhang, Roy Fan > > > <roy.fan.zh...@intel.com>; > > > > > > > Kusztal, > > > > > > > > > ArkadiuszX <arkadiuszx.kusz...@intel.com>; sta...@dpdk.org > > > > > > > > > Subject: [PATCH 2/3] examples/fips_validation: ignore \r in > input > > > files > > > > > > > > > > > > > > > > > > Some test vectors contain '\r' before '\n' (see link). Ignore > them. > > > > > > > > > > > > > > > > > > Link: https://www.openssl.org/docs/fips/testvectors-linux- > 2007- > > > 10- > > > > > > > 10.tar.gz > > > > > > > > > Fixes: 3d0fad56b74a ("examples/fips_validation: add crypto > FIPS > > > > > > > application") > > > > > > > > > Cc: sta...@dpdk.org > > > > > > > > > > > > > > > > > > Signed-off-by: Olivier Matz <olivier.m...@6wind.com> > > > > > > > > > --- > > > > > > > > > examples/fips_validation/fips_validation.c | 2 ++ > > > > > > > > > 1 file changed, 2 insertions(+) > > > > > > > > > > > > > > > > > > diff --git a/examples/fips_validation/fips_validation.c > > > > > > > > > b/examples/fips_validation/fips_validation.c > > > > > > > > > index 13f763c9aa..858f581ba3 100644 > > > > > > > > > --- a/examples/fips_validation/fips_validation.c > > > > > > > > > +++ b/examples/fips_validation/fips_validation.c > > > > > > > > > @@ -33,6 +33,8 @@ get_file_line(void) > > > > > > > > > > > > > > > > > > if (loc >= MAX_LINE_CHAR - 1) > > > > > > > > > return -ENOMEM; > > > > > > > > > + if (c == '\r') > > > > > > > > > + continue; > > > > > > > > > if (c == '\n') > > > > > > > > > break; > > > > > > > > > line[loc++] = c; > > > > > > > > > -- > > > > > > > > > > > > > > > > > > > > > > > > The patch looks ok but the test file link you provided in the > > > > > > > > patch > is > > > > > CAVS > > > > > > > > 5.3. > > > > > > > > > > > > > > > > As mentioned in > > > > > > > > > https://doc.dpdk.org/guides/sample_app_ug/fips_validation.html, > > > the > > > > > > > supported > > > > > > > > CAVS supported version is 21.0 (not latest one by newer than > 5.3). > > > In > > > > > CAVS > > > > > > > > 21.0 test files there is no '\r' before '\n' (I suppose this is > > > > > > > > for > > > Windows > > > > > > > > right). > > > > > > > > > > > > > > Thank you for your feedback. > > > > > > > > > > > > > > I'm ok to drop this patch from the patchset if you feel it's > > > > > > > useless, > or > > > > > > > I can update the commit log with the information you provide, to > > > clarify > > > > > > > that it should not happen with the supported version of CAVS. > > > > > > > > > > > > > > Please let me know what you prefer. > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > Olivier
