Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
From:   Yusong Gao <a869920004@gmail.com>
To:     juergh@proton.me, jarkko@kernel.org
Cc:     a869920004@gmail.com, davem@davemloft.net, dhowells@redhat.com,
        dimitri.ledkov@canonical.com, dwmw2@infradead.org,
        herbert@gondor.apana.org.au, keyrings@vger.kernel.org,
        linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org,
        lists@sapience.com, zohar@linux.ibm.com
Subject: [PATCH v3] sign-file: Fix incorrect return values check
Date:   Mon, 27 Nov 2023 02:57:37 +0000
Message-Id: <20231127025737.451718-1-a869920004@gmail.com>
In-Reply-To: <20231122082050.7eeea7bd@smeagol>
References: <20231122082050.7eeea7bd@smeagol>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Nov 22, 2023 at 3:21 PM Juerg Haefliger <juergh@proton.me>=0D
wrote:=0D
>=0D
> On Tue, 21 Nov 2023 03:40:44 +0000=0D
> "Yusong Gao" <a869920004@gmail.com> wrote:=0D
>=0D
> > There are some wrong return values check in sign-file when call=0D
> > OpenSSL=0D
> > API. The ERR() check cond is wrong because of the program only check=0D
> > the=0D
> > return value is < 0 instead of <=3D 0. For example:=0D
> > 1. CMS_final() return 1 for success or 0 for failure.=0D
> > 2. i2d_CMS_bio_stream() returns 1 for success or 0 for failure.=0D
> > 3. i2d_TYPEbio() return 1 for success and 0 for failure.=0D
> > 4. BIO_free() return 1 for success and 0 for failure.=0D
>=0D
> Good catch! In this case I'd probably be more strict and check for '!=3D=
=0D
> 1'.=0D
> See below.=0D
>=0D
> ...Juerg=0D
>=0D
>=0D
> > Link: https://www.openssl.org/docs/manmaster/man3/=0D
> > Fixes: e5a2e3c84782 ("scripts/sign-file.c: Add support for signing=0D
> > with a raw signature")=0D
> >=0D
> > Signed-off-by: Yusong Gao <a869920004@gmail.com>=0D
> > ---=0D
> > V1, V2: Clarify the description of git message.=0D
> > ---=0D
> > =C2=A0scripts/sign-file.c | 12 ++++++------=0D
> > =C2=A01 file changed, 6 insertions(+), 6 deletions(-)=0D
> >=0D
> > diff --git a/scripts/sign-file.c b/scripts/sign-file.c=0D
> > index 598ef5465f82..dcebbcd6bebd 100644=0D
> > --- a/scripts/sign-file.c=0D
> > +++ b/scripts/sign-file.c=0D
> > @@ -322,7 +322,7 @@ int main(int argc, char **argv)=0D
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0CMS_NOSMIMECAP | use=
_keyid |=0D
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0use_signed_attrs),=0D
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "CMS_add=
1_signer");=0D
> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ERR(CMS_final(cms, bm, NULL=
, CMS_NOCERTS | CMS_BINARY)=0D
> > < 0,=0D
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ERR(CMS_final(cms, bm, NULL=
, CMS_NOCERTS | CMS_BINARY)=0D
> > <=3D 0,=0D
>=0D
> ERR(CMS_final(cms, bm, NULL, CMS_NOCERTS | CMS_BINARY) !=3D 1,=0D
>=0D
>=0D
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "CMS_fin=
al");=0D
> >=0D
> > =C2=A0#else=0D
> > @@ -341,10 +341,10 @@ int main(int argc, char **argv)=0D
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 b =3D BIO_new_file(sig_file_name, "wb");=0D
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 ERR(!b, "%s", sig_file_name);=0D
> > =C2=A0#ifndef USE_PKCS7=0D
> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 ERR(i2d_CMS_bio_stream(b, cms, NULL, 0) < 0,=0D
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 ERR(i2d_CMS_bio_stream(b, cms, NULL, 0) <=3D 0,=0D
>=0D
> ERR(i2d_CMS_bio_stream(b, cms, NULL, 0) !=3D 1,=0D
>=0D
>=0D
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 "%s", sig_file_name);=0D
> > =C2=A0#else=0D
> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 ERR(i2d_PKCS7_bio(b, pkcs7) < 0,=0D
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 ERR(i2d_PKCS7_bio(b, pkcs7) <=3D 0,=0D
>=0D
> ERR(i2d_PKCS7_bio(b, pkcs7) !=3D 1,=0D
>=0D
>=0D
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 "%s", sig_file_name);=0D
> > =C2=A0#endif=0D
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 BIO_free(b);=0D
> > @@ -374,9 +374,9 @@ int main(int argc, char **argv)=0D
> >=0D
> > =C2=A0 =C2=A0 =C2=A0 if (!raw_sig) {=0D
> > =C2=A0#ifndef USE_PKCS7=0D
> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ERR(i2d_CMS_bio_stream(bd, =
cms, NULL, 0) < 0, "%s",=0D
> > dest_name);=0D
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ERR(i2d_CMS_bio_stream(bd, =
cms, NULL, 0) <=3D 0, "%s",=0D
> > dest_name);=0D
>=0D
>=0D
> ERR(i2d_CMS_bio_stream(bd, cms, NULL, 0) !=3D 1, "%s", dest_name);=0D
>=0D
>=0D
> > =C2=A0#else=0D
> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ERR(i2d_PKCS7_bio(bd, pkcs7=
) < 0, "%s", dest_name);=0D
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ERR(i2d_PKCS7_bio(bd, pkcs7=
) <=3D 0, "%s", dest_name);=0D
>=0D
> ERR(i2d_PKCS7_bio(bd, pkcs7) !=3D 1, "%s", dest_name);=0D
>=0D
>=0D
> > =C2=A0#endif=0D
> > =C2=A0 =C2=A0 =C2=A0 } else {=0D
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BIO *b;=0D
> > @@ -396,7 +396,7 @@ int main(int argc, char **argv)=0D
> > =C2=A0 =C2=A0 =C2=A0 ERR(BIO_write(bd, &sig_info, sizeof(sig_info)) < 0=
, "%s",=0D
> > dest_name);=0D
> > =C2=A0 =C2=A0 =C2=A0 ERR(BIO_write(bd, magic_number, sizeof(magic_numbe=
r) - 1) < 0,=0D
> > "%s", dest_name);=0D
> >=0D
> > - =C2=A0 =C2=A0 ERR(BIO_free(bd) < 0, "%s", dest_name);=0D
> > + =C2=A0 =C2=A0 ERR(BIO_free(bd) <=3D 0, "%s", dest_name);=0D
>=0D
> ERR(BIO_free(bd) !=3D 1, "%s", dest_name);=0D
>=0D
>=0D
> >=0D
> > =C2=A0 =C2=A0 =C2=A0 /* Finally, if we're signing in place, replace the=
 original.=0D
> > */=0D
> > =C2=A0 =C2=A0 =C2=A0 if (replace_orig)=0D
> > --=0D
> > 2.34.1=0D
> >=0D
>=0D
=0D
Agreed. Will do.=0D
Thanks for your review.=0D
=0D
=0D
BR, Yusong Gao=0D