| Message ID | 20190219072618.23048-1-yuehaibing@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [-next] tpm: change the return type of calc_tpm2_event_size to size_t | expand |
On Tue, Feb 19, 2019 at 03:26:18PM +0800, YueHaibing wrote: > calc_tpm2_event_size return size of the event which type is > size_t, If it is an invalid event, returns 0. And all the > caller use a size_t variable to check the return value, so > no need to convert to the return value type to int. > > Signed-off-by: YueHaibing <yuehaibing@huawei.com> I don't see this patch make any value. /Jarkko
On 2019/2/19 16:59, Jarkko Sakkinen wrote: > On Tue, Feb 19, 2019 at 03:26:18PM +0800, YueHaibing wrote: >> calc_tpm2_event_size return size of the event which type is >> size_t, If it is an invalid event, returns 0. And all the >> caller use a size_t variable to check the return value, so >> no need to convert to the return value type to int. >> >> Signed-off-by: YueHaibing <yuehaibing@huawei.com> > > I don't see this patch make any value. calc_tpm2_event_size return size_t 'size' which derive from u32 'event_field->event_size', convert it to int may result in truncation. > > /Jarkko > > . >
On Tue, Feb 19, 2019 at 05:15:47PM +0800, YueHaibing wrote: > On 2019/2/19 16:59, Jarkko Sakkinen wrote: > > On Tue, Feb 19, 2019 at 03:26:18PM +0800, YueHaibing wrote: > >> calc_tpm2_event_size return size of the event which type is > >> size_t, If it is an invalid event, returns 0. And all the > >> caller use a size_t variable to check the return value, so > >> no need to convert to the return value type to int. > >> > >> Signed-off-by: YueHaibing <yuehaibing@huawei.com> > > > > I don't see this patch make any value. > > calc_tpm2_event_size return size_t 'size' which derive from u32 'event_field->event_size', > > convert it to int may result in truncation. Two remarks: - Your real name is formatted incorrectly. It should be like "Yue Haibig" as shown in [1]. - If there is no actual regression, this change is useless. [1] https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html /Jarkko
On 2019/2/19 18:35, Jarkko Sakkinen wrote: > On Tue, Feb 19, 2019 at 05:15:47PM +0800, YueHaibing wrote: >> On 2019/2/19 16:59, Jarkko Sakkinen wrote: >>> On Tue, Feb 19, 2019 at 03:26:18PM +0800, YueHaibing wrote: >>>> calc_tpm2_event_size return size of the event which type is >>>> size_t, If it is an invalid event, returns 0. And all the >>>> caller use a size_t variable to check the return value, so >>>> no need to convert to the return value type to int. >>>> >>>> Signed-off-by: YueHaibing <yuehaibing@huawei.com> >>> >>> I don't see this patch make any value. >> >> calc_tpm2_event_size return size_t 'size' which derive from u32 'event_field->event_size', >> >> convert it to int may result in truncation. > > Two remarks: > > - Your real name is formatted incorrectly. It should be like "Yue Haibig" > as shown in [1]. > - If there is no actual regression, this change is useless. Ok, Thank you. > > [1] https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html > > /Jarkko > > . >
On Tue, Feb 19, 2019 at 12:35:32PM +0200, Jarkko Sakkinen wrote: > On Tue, Feb 19, 2019 at 05:15:47PM +0800, YueHaibing wrote: > > On 2019/2/19 16:59, Jarkko Sakkinen wrote: > > > On Tue, Feb 19, 2019 at 03:26:18PM +0800, YueHaibing wrote: > > >> calc_tpm2_event_size return size of the event which type is > > >> size_t, If it is an invalid event, returns 0. And all the > > >> caller use a size_t variable to check the return value, so > > >> no need to convert to the return value type to int. > > >> > > >> Signed-off-by: YueHaibing <yuehaibing@huawei.com> > > > > > > I don't see this patch make any value. > > > > calc_tpm2_event_size return size_t 'size' which derive from u32 'event_field->event_size', > > > > convert it to int may result in truncation. > > Two remarks: > > - Your real name is formatted incorrectly. It should be like "Yue Haibig" > as shown in [1]. > - If there is no actual regression, this change is useless. Clarity of the code is a laudible goal - not everything is about fixing bugs or adding features. It is much easier to write the code properly than to try and prove that event_size can not exceed INT_MAX. Jason
On Tue, Feb 19, 2019 at 09:34:56AM -0700, Jason Gunthorpe wrote: > > - Your real name is formatted incorrectly. It should be like "Yue Haibig" > > as shown in [1]. > > - If there is no actual regression, this change is useless. > > Clarity of the code is a laudible goal - not everything is about > fixing bugs or adding features. > > It is much easier to write the code properly than to try and prove > that event_size can not exceed INT_MAX. I think it is actually a regression in 4d23cc323cdb. Should carry a fixes tag. I did not quite undersand what commit message was saying. What it should would just """ tpm: Fix the type of the return value in calc_tpm2_event_size() calc_tpm2_event_size() has an invalid signature because it returns a 'size_t' where as its signature says that it returns 'int'. """ Should be also Cc'd to linux-security-module and have cc to stable. /Jarkko
On 2019/2/20 1:31, Jarkko Sakkinen wrote: > On Tue, Feb 19, 2019 at 09:34:56AM -0700, Jason Gunthorpe wrote: >>> - Your real name is formatted incorrectly. It should be like "Yue Haibig" >>> as shown in [1]. >>> - If there is no actual regression, this change is useless. >> >> Clarity of the code is a laudible goal - not everything is about >> fixing bugs or adding features. >> >> It is much easier to write the code properly than to try and prove >> that event_size can not exceed INT_MAX. > > I think it is actually a regression in 4d23cc323cdb. Should carry a > fixes tag. I did not quite undersand what commit message was saying. > What it should would just > > """ > tpm: Fix the type of the return value in calc_tpm2_event_size() > > calc_tpm2_event_size() has an invalid signature because it returns a 'size_t' > where as its signature says that it returns 'int'. > """ > > Should be also Cc'd to linux-security-module and have cc to stable. Ok, I will fix it in v2. > > /Jarkko > > . >
diff --git a/drivers/char/tpm/eventlog/tpm2.c b/drivers/char/tpm/eventlog/tpm2.c index d8b7713..f824563 100644 --- a/drivers/char/tpm/eventlog/tpm2.c +++ b/drivers/char/tpm/eventlog/tpm2.c @@ -37,8 +37,8 @@ * * Returns size of the event. If it is an invalid event, returns 0. */ -static int calc_tpm2_event_size(struct tcg_pcr_event2_head *event, - struct tcg_pcr_event *event_header) +static size_t calc_tpm2_event_size(struct tcg_pcr_event2_head *event, + struct tcg_pcr_event *event_header) { struct tcg_efi_specid_event_head *efispecid; struct tcg_event_field *event_field;
calc_tpm2_event_size return size of the event which type is size_t, If it is an invalid event, returns 0. And all the caller use a size_t variable to check the return value, so no need to convert to the return value type to int. Signed-off-by: YueHaibing <yuehaibing@huawei.com> --- drivers/char/tpm/eventlog/tpm2.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)