[1/1] avb2.0: add get_size_of_partition()

Message ID 1531138554-7429-1-git-send-email-igor.opaniuk@linaro.org
State Superseded
Headers show
Series
  • [1/1] avb2.0: add get_size_of_partition()
Related show

Commit Message

Igor Opaniuk July 9, 2018, 12:15 p.m.
Implement get_size_of_partition() operation,
which is required by the latest upstream libavb [1].

[1] https://android.googlesource.com/platform/external/avb/+/master/README.md

Signed-off-by: Igor Opaniuk <igor.opaniuk@linaro.org>
---
 common/avb_verify.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

Comments

Sam Protsenko July 9, 2018, 2:52 p.m. | #1
On Mon, Jul 9, 2018 at 3:15 PM, Igor Opaniuk <igor.opaniuk@linaro.org> wrote:
> Implement get_size_of_partition() operation,
> which is required by the latest upstream libavb [1].
>
> [1] https://android.googlesource.com/platform/external/avb/+/master/README.md
>
> Signed-off-by: Igor Opaniuk <igor.opaniuk@linaro.org>
> ---

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

>  common/avb_verify.c | 33 ++++++++++++++++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/common/avb_verify.c b/common/avb_verify.c
> index f9a00f8..5eabab0 100644
> --- a/common/avb_verify.c
> +++ b/common/avb_verify.c
> @@ -699,6 +699,37 @@ static AvbIOResult get_unique_guid_for_partition(AvbOps *ops,
>  }
>
>  /**
> + * get_size_of_partition() - gets the size of a partition identified
> + * by a string name
> + *
> + * @ops: contains AVB ops handlers
> + * @partition: partition name (NUL-terminated UTF-8 string)
> + * @out_size_num_bytes: returns the value of a partition size
> + *
> + * @return:
> + *      AVB_IO_RESULT_OK, on success (GUID found)
> + *      AVB_IO_RESULT_ERROR_IO, out_size_num_bytes is NULL
> + *      AVB_IO_RESULT_ERROR_NO_SUCH_PARTITION, if partition was not found
> + */
> +static AvbIOResult get_size_of_partition(AvbOps *ops,
> +                                        const char *partition,
> +                                        u64 *out_size_num_bytes)
> +{
> +       struct mmc_part *part;
> +
> +       if (!out_size_num_bytes)
> +               return AVB_IO_RESULT_ERROR_IO;
> +
> +       part = get_partition(ops, partition);
> +       if (!part)
> +               return AVB_IO_RESULT_ERROR_NO_SUCH_PARTITION;
> +
> +       *out_size_num_bytes = part->info.blksz * part->info.size;
> +
> +       return AVB_IO_RESULT_OK;
> +}
> +
> +/**
>   * ============================================================================
>   * AVB2.0 AvbOps alloc/initialisation/free
>   * ============================================================================
> @@ -721,7 +752,7 @@ AvbOps *avb_ops_alloc(int boot_device)
>         ops_data->ops.read_is_device_unlocked = read_is_device_unlocked;
>         ops_data->ops.get_unique_guid_for_partition =
>                 get_unique_guid_for_partition;
> -
> +       ops_data->ops.get_size_of_partition = get_size_of_partition;
>         ops_data->mmc_dev = boot_device;
>
>         return &ops_data->ops;
> --
> 2.7.4
>
Andrew F. Davis July 9, 2018, 3:21 p.m. | #2
On 07/09/2018 09:52 AM, Sam Protsenko wrote:
> On Mon, Jul 9, 2018 at 3:15 PM, Igor Opaniuk <igor.opaniuk@linaro.org> wrote:
>> Implement get_size_of_partition() operation,
>> which is required by the latest upstream libavb [1].
>>
>> [1] https://android.googlesource.com/platform/external/avb/+/master/README.md
>>


I may have missed it, where in here do we need this information? I looks
to be passed in on the command line for most ops. Has a new function
been added?


>> Signed-off-by: Igor Opaniuk <igor.opaniuk@linaro.org>
>> ---
> 
> Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>
> 
>>  common/avb_verify.c | 33 ++++++++++++++++++++++++++++++++-
>>  1 file changed, 32 insertions(+), 1 deletion(-)
>>
>> diff --git a/common/avb_verify.c b/common/avb_verify.c
>> index f9a00f8..5eabab0 100644
>> --- a/common/avb_verify.c
>> +++ b/common/avb_verify.c
>> @@ -699,6 +699,37 @@ static AvbIOResult get_unique_guid_for_partition(AvbOps *ops,
>>  }
>>
>>  /**
>> + * get_size_of_partition() - gets the size of a partition identified
>> + * by a string name
>> + *
>> + * @ops: contains AVB ops handlers
>> + * @partition: partition name (NUL-terminated UTF-8 string)
>> + * @out_size_num_bytes: returns the value of a partition size
>> + *
>> + * @return:
>> + *      AVB_IO_RESULT_OK, on success (GUID found)
>> + *      AVB_IO_RESULT_ERROR_IO, out_size_num_bytes is NULL


This does not seems to be the right error code for this, this implies a
hardware error, maybe AVB_IO_RESULT_ERROR_INSUFFICIENT_SPACE is a better
choice? 'out_size_num_bytes' is a buffer in a way (to 8 bytes)..

Andrew


>> + *      AVB_IO_RESULT_ERROR_NO_SUCH_PARTITION, if partition was not found
>> + */
>> +static AvbIOResult get_size_of_partition(AvbOps *ops,
>> +                                        const char *partition,
>> +                                        u64 *out_size_num_bytes)
>> +{
>> +       struct mmc_part *part;
>> +
>> +       if (!out_size_num_bytes)
>> +               return AVB_IO_RESULT_ERROR_IO;
>> +
>> +       part = get_partition(ops, partition);
>> +       if (!part)
>> +               return AVB_IO_RESULT_ERROR_NO_SUCH_PARTITION;
>> +
>> +       *out_size_num_bytes = part->info.blksz * part->info.size;
>> +
>> +       return AVB_IO_RESULT_OK;
>> +}
>> +
>> +/**
>>   * ============================================================================
>>   * AVB2.0 AvbOps alloc/initialisation/free
>>   * ============================================================================
>> @@ -721,7 +752,7 @@ AvbOps *avb_ops_alloc(int boot_device)
>>         ops_data->ops.read_is_device_unlocked = read_is_device_unlocked;
>>         ops_data->ops.get_unique_guid_for_partition =
>>                 get_unique_guid_for_partition;
>> -
>> +       ops_data->ops.get_size_of_partition = get_size_of_partition;
>>         ops_data->mmc_dev = boot_device;
>>
>>         return &ops_data->ops;
>> --
>> 2.7.4
>>
Eugeniu Rosca July 9, 2018, 3:33 p.m. | #3
On Mon, Jul 09, 2018 at 03:15:54PM +0300, Igor Opaniuk wrote:
> Implement get_size_of_partition() operation,
> which is required by the latest upstream libavb [1].
> 
> [1] https://android.googlesource.com/platform/external/avb/+/master/README.md
> 
> Signed-off-by: Igor Opaniuk <igor.opaniuk@linaro.org>
> ---
>  common/avb_verify.c | 33 ++++++++++++++++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)


Hi Igor,

Is there a way to play with and smoke-test libavb on sandbox?
FWIW currently, independently on this patch, menuconfig interface allows me
to select CONFIG_LIBAVB, but then U-Boot compilation fails as below:

$ make defconfig
$ make menuconfig => select LIBAVB=y
$ make

---8<---
In file included from common/avb_verify.c:7:0:
include/avb_verify.h: In function ‘get_sector_buf_size’:
include/avb_verify.h:70:17: error: ‘CONFIG_FASTBOOT_BUF_SIZE’ undeclared (first use in this function); did you mean ‘CONFIG_PRE_CON_BUF_SZ’?
  return (size_t)CONFIG_FASTBOOT_BUF_SIZE;
                 ^~~~~~~~~~~~~~~~~~~~~~~~
                 CONFIG_PRE_CON_BUF_SZ
include/avb_verify.h:70:17: note: each undeclared identifier is reported only once for each function it appears in
include/avb_verify.h: In function ‘get_sector_buf’:
include/avb_verify.h:75:17: error: ‘CONFIG_FASTBOOT_BUF_ADDR’ undeclared (first use in this function); did you mean ‘CONFIG_PRE_CON_BUF_ADDR’?
  return (void *)CONFIG_FASTBOOT_BUF_ADDR;
                 ^~~~~~~~~~~~~~~~~~~~~~~~
                 CONFIG_PRE_CON_BUF_ADDR
  CC      env/attr.o
---8<---

If there were an easy way to smoke-test libavb on sandbox, I think
people like me would be more motivated to provide their Tested-by
in addition to their review (just my feeling).

Best regards,
Eugeniu.
Igor Opaniuk July 9, 2018, 4:15 p.m. | #4
Hi Eugeniu,

Thanks for reporting this issue,
LIBAVB should depend on CONFIG_FASTBOOT, as fastboot buffer is re-used
(which initially
is used in the fastboot protocol for downloads) for
mmc read/write AvbOps (and buffer size is configured by setting
CONFIG_FASTBOOT_BUF_ADDR
and CONFIG_FASTBOOT_BUF_SIZE)

The problem is that both CONFIG_FASTBOOT_BUF_ADDR and CONFIG_FASTBOOT_BUF_SIZE
are defined for most platforms, and this is how I missed this issue.

Will fix today and re-test,
Thanks

On 9 July 2018 at 18:33, Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> On Mon, Jul 09, 2018 at 03:15:54PM +0300, Igor Opaniuk wrote:
>> Implement get_size_of_partition() operation,
>> which is required by the latest upstream libavb [1].
>>
>> [1] https://android.googlesource.com/platform/external/avb/+/master/README.md
>>
>> Signed-off-by: Igor Opaniuk <igor.opaniuk@linaro.org>
>> ---
>>  common/avb_verify.c | 33 ++++++++++++++++++++++++++++++++-
>>  1 file changed, 32 insertions(+), 1 deletion(-)
>
>
> Hi Igor,
>
> Is there a way to play with and smoke-test libavb on sandbox?
> FWIW currently, independently on this patch, menuconfig interface allows me
> to select CONFIG_LIBAVB, but then U-Boot compilation fails as below:
>
> $ make defconfig
> $ make menuconfig => select LIBAVB=y
> $ make
>
> ---8<---
> In file included from common/avb_verify.c:7:0:
> include/avb_verify.h: In function ‘get_sector_buf_size’:
> include/avb_verify.h:70:17: error: ‘CONFIG_FASTBOOT_BUF_SIZE’ undeclared (first use in this function); did you mean ‘CONFIG_PRE_CON_BUF_SZ’?
>   return (size_t)CONFIG_FASTBOOT_BUF_SIZE;
>                  ^~~~~~~~~~~~~~~~~~~~~~~~
>                  CONFIG_PRE_CON_BUF_SZ
> include/avb_verify.h:70:17: note: each undeclared identifier is reported only once for each function it appears in
> include/avb_verify.h: In function ‘get_sector_buf’:
> include/avb_verify.h:75:17: error: ‘CONFIG_FASTBOOT_BUF_ADDR’ undeclared (first use in this function); did you mean ‘CONFIG_PRE_CON_BUF_ADDR’?
>   return (void *)CONFIG_FASTBOOT_BUF_ADDR;
>                  ^~~~~~~~~~~~~~~~~~~~~~~~
>                  CONFIG_PRE_CON_BUF_ADDR
>   CC      env/attr.o
> ---8<---
>
> If there were an easy way to smoke-test libavb on sandbox, I think
> people like me would be more motivated to provide their Tested-by
> in addition to their review (just my feeling).
>
> Best regards,
> Eugeniu.
Igor Opaniuk July 13, 2018, 5:09 p.m. | #5
Hi Andrew,
Sorry I missed your message.

On 9 July 2018 at 18:21, Andrew F. Davis <afd@ti.com> wrote:
> On 07/09/2018 09:52 AM, Sam Protsenko wrote:
>> On Mon, Jul 9, 2018 at 3:15 PM, Igor Opaniuk <igor.opaniuk@linaro.org> wrote:
>>> Implement get_size_of_partition() operation,
>>> which is required by the latest upstream libavb [1].
>>>
>>> [1] https://android.googlesource.com/platform/external/avb/+/master/README.md
>>>
>
>
> I may have missed it, where in here do we need this information? I looks
> to be passed in on the command line for most ops. Has a new function
> been added?

right, it was introduced in 417e8133af46 ("libavb: Load entire
partition if |allow_verification_error| is true.").
When I included the latest libavb for the avb v2 patch series (in v1
the was out-dated version, AFAIK ~1y old),
I noticed this new Avb operation, although it didn't have any impact
on avb verify functionality.
(I was receiving just a warning that no get_size_of_partition is set
in AvbOps structure)

So I decided to introduce the implementation of this function in a
separate patch.

>
>>> Signed-off-by: Igor Opaniuk <igor.opaniuk@linaro.org>
>>> ---
>>
>> Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>
>>
>>>  common/avb_verify.c | 33 ++++++++++++++++++++++++++++++++-
>>>  1 file changed, 32 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/common/avb_verify.c b/common/avb_verify.c
>>> index f9a00f8..5eabab0 100644
>>> --- a/common/avb_verify.c
>>> +++ b/common/avb_verify.c
>>> @@ -699,6 +699,37 @@ static AvbIOResult get_unique_guid_for_partition(AvbOps *ops,
>>>  }
>>>
>>>  /**
>>> + * get_size_of_partition() - gets the size of a partition identified
>>> + * by a string name
>>> + *
>>> + * @ops: contains AVB ops handlers
>>> + * @partition: partition name (NUL-terminated UTF-8 string)
>>> + * @out_size_num_bytes: returns the value of a partition size
>>> + *
>>> + * @return:
>>> + *      AVB_IO_RESULT_OK, on success (GUID found)
>>> + *      AVB_IO_RESULT_ERROR_IO, out_size_num_bytes is NULL
>
>
> This does not seems to be the right error code for this, this implies a
> hardware error, maybe AVB_IO_RESULT_ERROR_INSUFFICIENT_SPACE is a better
> choice? 'out_size_num_bytes' is a buffer in a way (to 8 bytes).

Frankly, I chose the most "abstract" (if I can say that) error code,
as there is no any in AVB that resembles POSIX EINVAL.
Regarding AVB_IO_RESULT_ERROR_INSUFFICIENT_SPACE, which
"is returned if a buffer is too small for the requested operation",
I'm not sure that it's proper error either.

There is a question spinning in my mind "should we do any param verification?",
as only libavb is using these functions,
and no theoretical case is possible when an invalid value is provided.

>
> Andrew
>
>
>>> + *      AVB_IO_RESULT_ERROR_NO_SUCH_PARTITION, if partition was not found
>>> + */
>>> +static AvbIOResult get_size_of_partition(AvbOps *ops,
>>> +                                        const char *partition,
>>> +                                        u64 *out_size_num_bytes)
>>> +{
>>> +       struct mmc_part *part;
>>> +
>>> +       if (!out_size_num_bytes)
>>> +               return AVB_IO_RESULT_ERROR_IO;
>>> +
>>> +       part = get_partition(ops, partition);
>>> +       if (!part)
>>> +               return AVB_IO_RESULT_ERROR_NO_SUCH_PARTITION;
>>> +
>>> +       *out_size_num_bytes = part->info.blksz * part->info.size;
>>> +
>>> +       return AVB_IO_RESULT_OK;
>>> +}
>>> +
>>> +/**
>>>   * ============================================================================
>>>   * AVB2.0 AvbOps alloc/initialisation/free
>>>   * ============================================================================
>>> @@ -721,7 +752,7 @@ AvbOps *avb_ops_alloc(int boot_device)
>>>         ops_data->ops.read_is_device_unlocked = read_is_device_unlocked;
>>>         ops_data->ops.get_unique_guid_for_partition =
>>>                 get_unique_guid_for_partition;
>>> -
>>> +       ops_data->ops.get_size_of_partition = get_size_of_partition;
>>>         ops_data->mmc_dev = boot_device;
>>>
>>>         return &ops_data->ops;
>>> --
>>> 2.7.4
>>>
Andrew F. Davis July 13, 2018, 5:31 p.m. | #6
On 07/13/2018 12:09 PM, Igor Opaniuk wrote:
> Hi Andrew,
> Sorry I missed your message.
> 
> On 9 July 2018 at 18:21, Andrew F. Davis <afd@ti.com> wrote:
>> On 07/09/2018 09:52 AM, Sam Protsenko wrote:
>>> On Mon, Jul 9, 2018 at 3:15 PM, Igor Opaniuk <igor.opaniuk@linaro.org> wrote:
>>>> Implement get_size_of_partition() operation,
>>>> which is required by the latest upstream libavb [1].
>>>>
>>>> [1] https://android.googlesource.com/platform/external/avb/+/master/README.md
>>>>
>>
>>
>> I may have missed it, where in here do we need this information? I looks
>> to be passed in on the command line for most ops. Has a new function
>> been added?
> 
> right, it was introduced in 417e8133af46 ("libavb: Load entire
> partition if |allow_verification_error| is true.").
> When I included the latest libavb for the avb v2 patch series (in v1
> the was out-dated version, AFAIK ~1y old),
> I noticed this new Avb operation, although it didn't have any impact
> on avb verify functionality.
> (I was receiving just a warning that no get_size_of_partition is set
> in AvbOps structure)
> 
> So I decided to introduce the implementation of this function in a
> separate patch.
> 
>>
>>>> Signed-off-by: Igor Opaniuk <igor.opaniuk@linaro.org>
>>>> ---
>>>
>>> Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>
>>>
>>>>  common/avb_verify.c | 33 ++++++++++++++++++++++++++++++++-
>>>>  1 file changed, 32 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/common/avb_verify.c b/common/avb_verify.c
>>>> index f9a00f8..5eabab0 100644
>>>> --- a/common/avb_verify.c
>>>> +++ b/common/avb_verify.c
>>>> @@ -699,6 +699,37 @@ static AvbIOResult get_unique_guid_for_partition(AvbOps *ops,
>>>>  }
>>>>
>>>>  /**
>>>> + * get_size_of_partition() - gets the size of a partition identified
>>>> + * by a string name
>>>> + *
>>>> + * @ops: contains AVB ops handlers
>>>> + * @partition: partition name (NUL-terminated UTF-8 string)
>>>> + * @out_size_num_bytes: returns the value of a partition size
>>>> + *
>>>> + * @return:
>>>> + *      AVB_IO_RESULT_OK, on success (GUID found)
>>>> + *      AVB_IO_RESULT_ERROR_IO, out_size_num_bytes is NULL
>>
>>
>> This does not seems to be the right error code for this, this implies a
>> hardware error, maybe AVB_IO_RESULT_ERROR_INSUFFICIENT_SPACE is a better
>> choice? 'out_size_num_bytes' is a buffer in a way (to 8 bytes).
> 
> Frankly, I chose the most "abstract" (if I can say that) error code,
> as there is no any in AVB that resembles POSIX EINVAL.


But it is not abstract or generic, it is very specific:

"is returned if the underlying hardware encountered an I/O error."

If no I/O error has occurred, you should not be returning this error code.


> Regarding AVB_IO_RESULT_ERROR_INSUFFICIENT_SPACE, which
> "is returned if a buffer is too small for the requested operation",
> I'm not sure that it's proper error either.
> 


Neither are perfectly proper, but lets not make the user think their HW
is broken.


> There is a question spinning in my mind "should we do any param verification?",
> as only libavb is using these functions,
> and no theoretical case is possible when an invalid value is provided.
> 


Sanity checks never hurt, trust nobody, not even yourself.

For parameters input by user then of course verify. But parameters only
populated by other code, then it depends.

It matters what will happen when at some point when someone will mess up
the code and this function will get called with bad params. In this
particular case all that will happen is a null pointer exception will
get thrown, the dev will see it instantly and fix the code. So it's not
needed here.

The ones that really need it are ones that the bad parameter will
propagate silently and break somewhere else much later and may even make
it back in to production code bases if the problem it causes isn't
immediately obvious enough.

Andrew


>>
>> Andrew
>>
>>
>>>> + *      AVB_IO_RESULT_ERROR_NO_SUCH_PARTITION, if partition was not found
>>>> + */
>>>> +static AvbIOResult get_size_of_partition(AvbOps *ops,
>>>> +                                        const char *partition,
>>>> +                                        u64 *out_size_num_bytes)
>>>> +{
>>>> +       struct mmc_part *part;
>>>> +
>>>> +       if (!out_size_num_bytes)
>>>> +               return AVB_IO_RESULT_ERROR_IO;
>>>> +
>>>> +       part = get_partition(ops, partition);
>>>> +       if (!part)
>>>> +               return AVB_IO_RESULT_ERROR_NO_SUCH_PARTITION;
>>>> +
>>>> +       *out_size_num_bytes = part->info.blksz * part->info.size;
>>>> +
>>>> +       return AVB_IO_RESULT_OK;
>>>> +}
>>>> +
>>>> +/**
>>>>   * ============================================================================
>>>>   * AVB2.0 AvbOps alloc/initialisation/free
>>>>   * ============================================================================
>>>> @@ -721,7 +752,7 @@ AvbOps *avb_ops_alloc(int boot_device)
>>>>         ops_data->ops.read_is_device_unlocked = read_is_device_unlocked;
>>>>         ops_data->ops.get_unique_guid_for_partition =
>>>>                 get_unique_guid_for_partition;
>>>> -
>>>> +       ops_data->ops.get_size_of_partition = get_size_of_partition;
>>>>         ops_data->mmc_dev = boot_device;
>>>>
>>>>         return &ops_data->ops;
>>>> --
>>>> 2.7.4
>>>>
> 
> 
>

Patch

diff --git a/common/avb_verify.c b/common/avb_verify.c
index f9a00f8..5eabab0 100644
--- a/common/avb_verify.c
+++ b/common/avb_verify.c
@@ -699,6 +699,37 @@  static AvbIOResult get_unique_guid_for_partition(AvbOps *ops,
 }
 
 /**
+ * get_size_of_partition() - gets the size of a partition identified
+ * by a string name
+ *
+ * @ops: contains AVB ops handlers
+ * @partition: partition name (NUL-terminated UTF-8 string)
+ * @out_size_num_bytes: returns the value of a partition size
+ *
+ * @return:
+ *      AVB_IO_RESULT_OK, on success (GUID found)
+ *      AVB_IO_RESULT_ERROR_IO, out_size_num_bytes is NULL
+ *      AVB_IO_RESULT_ERROR_NO_SUCH_PARTITION, if partition was not found
+ */
+static AvbIOResult get_size_of_partition(AvbOps *ops,
+					 const char *partition,
+					 u64 *out_size_num_bytes)
+{
+	struct mmc_part *part;
+
+	if (!out_size_num_bytes)
+		return AVB_IO_RESULT_ERROR_IO;
+
+	part = get_partition(ops, partition);
+	if (!part)
+		return AVB_IO_RESULT_ERROR_NO_SUCH_PARTITION;
+
+	*out_size_num_bytes = part->info.blksz * part->info.size;
+
+	return AVB_IO_RESULT_OK;
+}
+
+/**
  * ============================================================================
  * AVB2.0 AvbOps alloc/initialisation/free
  * ============================================================================
@@ -721,7 +752,7 @@  AvbOps *avb_ops_alloc(int boot_device)
 	ops_data->ops.read_is_device_unlocked = read_is_device_unlocked;
 	ops_data->ops.get_unique_guid_for_partition =
 		get_unique_guid_for_partition;
-
+	ops_data->ops.get_size_of_partition = get_size_of_partition;
 	ops_data->mmc_dev = boot_device;
 
 	return &ops_data->ops;