Re: [PATCH kmod] libkmod-signature: implement pkcs7 parsing with openssl

From: Lucas De Marchi <lucas.demarchi@intel.com>
To: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
Cc: Lucas De Marchi <lucas.de.marchi@gmail.com>,
	linux-modules <linux-modules@vger.kernel.org>,
	ykaliuta@redhat.com
Subject: Re: [PATCH kmod] libkmod-signature: implement pkcs7 parsing with openssl
Date: Tue, 29 Jan 2019 08:50:10 -0800	[thread overview]
Message-ID: <20190129165010.kkcd3xrdgpf3uqkc@ldmartin-desk.jf.intel.com> (raw)
In-Reply-To: <xunybm3z94j6.fsf@redhat.com>

On Tue, Jan 29, 2019 at 11:50:05AM +0200, Yauheni Kaliuta wrote:
>Hi, Lucas!
>
>>>>>> On Mon, 28 Jan 2019 10:05:24 -0800, Lucas De Marchi  wrote:
> > On Sat, Jan 26, 2019 at 3:01 AM Yauheni Kaliuta
> > <yauheni.kaliuta@redhat.com> wrote:
> >>
>
>[...]
> >> >> +
> >> >> +       pvt->cms = cms;
> >> >> +       pvt->key_id = key_id_str;
> >> >> +       pvt->sno = sno_bn;
> >> >> +       sig_info->private = pvt;
> >>
> >> > why do you keep pvt around if the only thing you will do with
> >> > it later is to free it?
> >> > AFAICS the only thing that needs to remain around is the str
> >> > so we can free it after the user used it (because normal
> >> > signature is backed in memory by the mem object, while these
> >> > are openssl structs)
> >>
> >> I should keep them until kmod_module_get_info() makes the copies.
> >>
> >> cms is openssl struct
> >> sno_bn is allocated by openssl and must be freed later
> >> key_id_str is allocated here since the size in unknown in advance
> >> and must be freed later.
> >>
> >> Or what did I miss?
>
> > we could just duplicate the information that we want stored and keep
> > the openssl context contained
> > to just this function. I thought the only one would be key_str_id, but
> > missed that sig and signer
> > also need to have their backing object around.
>
>If I duplicate it here then without cleanup I'll have memory
>leak, no?

yes, my idea was to just leave it simpler and add a

if (pkcs7)
        free(key_id)

Lucas De Marchi

>
>In the old code they were pointers inside the module image and
>freed with the image itself.
>
>-- 
>WBR,
>Yauheni Kaliuta