Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 3 Jun 2019 13:08:45 -0700 (PDT)
From:   jaskarankhurana@linux.microsoft.com
To:     linux-kernel@vger.kernel.org,
        linux-security-module@vger.kernel.org, gmazyland@gmail.com
cc:     agk@redhat.com, snitzer@redhat.com, dm-deval@redhat.com,
        jmorris@namei.org, scottsh@microsoft.com
Subject: Re:[RFC 1/1] Add dm verity root hash pkcs7 sig validation.
In-Reply-To: <MN2PR21MB1200327819F4810E82AA6FF38A000@MN2PR21MB1200.namprd21.prod.outlook.com>
Message-ID: <alpine.LRH.2.21.1906031246001.64221@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.inter>
References: <MN2PR21MB1200327819F4810E82AA6FF38A000@MN2PR21MB1200.namprd21.prod.outlook.com>
User-Agent: Alpine 2.21 (LRH 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII; format=flowed
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On Tue, 21 May 2019, Milan Broz wrote:
> On 20/05/2019 23:54, Jaskaran Khurana wrote:
>> Adds in-kernel pkcs7 signature checking for the roothash of
>> the dm-verity hash tree.
>>
>>
>> Adds DM_VERITY_VERIFY_ROOTHASH_SIG_FORCE: roothash signature *must* be
>> specified for all dm verity volumes and verification must succeed prior
>> to creation of device mapper block device.
>
> I am not sure this is a good idea. If I understand it correctly, this will
> block creating another dm-verity mappings without PKCS7 signature, and these
> are used in many other environments and applications that could possibly
> run on that system later.
>
> (But I have no idea how to solve it better though :-)
>
> ...
>
>> +	/* Root hash signature is  a optional parameter*/
>> +	r = verity_verify_root_hash(root_hash_digest_to_validate,
>> +				    strlen(root_hash_digest_to_validate),
>> +				    verify_args.sig,
>> +				    verify_args.sig_size);
>> +	if (r < 0) {
>> +		ti->error = "Root hash verification failed";
>> +		goto bad;
>> +	}
>
> You are sending the PKCS7 signature as a (quite large) binary blob inside the mapping table.
>
> I am not sure if it is possible here (I guess so), but why not put this it kernel keyring
> and then just reference it from mapping table?
> (We use kernel keyring in libcryptsetup already for dm-crypt.)
>
> It will also solve an issue in userspace patch, when you are reading the signature
> file too late (devices can be suspended in that moment, so I would prefer to download
> sig file to keyring in advance, and then just reference it in mapping table).
>
> (I guess you will send merge request for veritysetup userspace part later.)

I have made the changes for passing the signature bytes using the keyring 
and I had sent an updated patch for the same last week. I have given a 
link to the veritysetup changes which I used to test this in the patch and I will 
cleanup and send those for review next.

Please take a look and provide code review feedback for the kernel 
changes.

>
> Milan
>
Regards,
Jaskaran