[PATCH] ACPI, APEI, Fix use resource_size

[PATCH] ACPI, APEI, Fix use resource_size

[Vasyl Gomonovych]

Use resource_size function on resource object
Underneath __request_region set res->end to start + n - 1
Lets use resourse_size to set value properly.

Signed-off-by: Vasyl Gomonovych <gomonovych@gmail.com>
---
 drivers/acpi/apei/apei-base.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/apei/apei-base.c b/drivers/acpi/apei/apei-base.c
index da370e1..af712a8 100644
--- a/drivers/acpi/apei/apei-base.c
+++ b/drivers/acpi/apei/apei-base.c
@@ -506,8 +506,7 @@ int apei_resources_request(struct apei_resources *resources,
 
 	rc = -EINVAL;
 	list_for_each_entry(res, &resources->iomem, list) {
-		r = request_mem_region(res->start, res->end - res->start,
-				       desc);
+		r = request_mem_region(res->start, resource_size(res), desc);
 		if (!r) {
 			pr_err(APEI_PFX
 		"Can not request [mem %#010llx-%#010llx] for %s registers\n",
@@ -519,7 +518,7 @@ int apei_resources_request(struct apei_resources *resources,
 	}
 
 	list_for_each_entry(res, &resources->ioport, list) {
-		r = request_region(res->start, res->end - res->start, desc);
+		r = request_region(res->start, resource_size(res), desc);
 		if (!r) {
 			pr_err(APEI_PFX
 		"Can not request [io  %#06llx-%#06llx] for %s registers\n",
@@ -542,14 +541,14 @@ int apei_resources_request(struct apei_resources *resources,
 	list_for_each_entry(res, &resources->ioport, list) {
 		if (res == res_bak)
 			break;
-		release_region(res->start, res->end - res->start);
+		release_region(res->start, resource_size(res));
 	}
 	res_bak = NULL;
 err_unmap_iomem:
 	list_for_each_entry(res, &resources->iomem, list) {
 		if (res == res_bak)
 			break;
-		release_mem_region(res->start, res->end - res->start);
+		release_mem_region(res->start, resource_size(res));
 	}
 arch_res_fini:
 	if (arch_apei_filter_addr)
@@ -566,9 +565,9 @@ void apei_resources_release(struct apei_resources *resources)
 	struct apei_res *res;
 
 	list_for_each_entry(res, &resources->iomem, list)
-		release_mem_region(res->start, res->end - res->start);
+		release_mem_region(res->start, resource_size(res));
 	list_for_each_entry(res, &resources->ioport, list)
-		release_region(res->start, res->end - res->start);
+		release_region(res->start, resource_size(res));
 
 	rc = apei_resources_sub(&apei_resources_all, resources);
 	if (rc)
-- 
1.9.1

Re: [PATCH] ACPI, APEI, Fix use resource_size

[Rafael J. Wysocki]

On Monday, December 11, 2017 4:44:31 PM CET Vasyl Gomonovych wrote:
> Use resource_size function on resource object
> Underneath __request_region set res->end to start + n - 1
> Lets use resourse_size to set value properly.
> 
> Signed-off-by: Vasyl Gomonovych <gomonovych@gmail.com>

Boris, what do you think?

> ---
>  drivers/acpi/apei/apei-base.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/acpi/apei/apei-base.c b/drivers/acpi/apei/apei-base.c
> index da370e1..af712a8 100644
> --- a/drivers/acpi/apei/apei-base.c
> +++ b/drivers/acpi/apei/apei-base.c
> @@ -506,8 +506,7 @@ int apei_resources_request(struct apei_resources *resources,
>  
>  	rc = -EINVAL;
>  	list_for_each_entry(res, &resources->iomem, list) {
> -		r = request_mem_region(res->start, res->end - res->start,
> -				       desc);
> +		r = request_mem_region(res->start, resource_size(res), desc);
>  		if (!r) {
>  			pr_err(APEI_PFX
>  		"Can not request [mem %#010llx-%#010llx] for %s registers\n",
> @@ -519,7 +518,7 @@ int apei_resources_request(struct apei_resources *resources,
>  	}
>  
>  	list_for_each_entry(res, &resources->ioport, list) {
> -		r = request_region(res->start, res->end - res->start, desc);
> +		r = request_region(res->start, resource_size(res), desc);
>  		if (!r) {
>  			pr_err(APEI_PFX
>  		"Can not request [io  %#06llx-%#06llx] for %s registers\n",
> @@ -542,14 +541,14 @@ int apei_resources_request(struct apei_resources *resources,
>  	list_for_each_entry(res, &resources->ioport, list) {
>  		if (res == res_bak)
>  			break;
> -		release_region(res->start, res->end - res->start);
> +		release_region(res->start, resource_size(res));
>  	}
>  	res_bak = NULL;
>  err_unmap_iomem:
>  	list_for_each_entry(res, &resources->iomem, list) {
>  		if (res == res_bak)
>  			break;
> -		release_mem_region(res->start, res->end - res->start);
> +		release_mem_region(res->start, resource_size(res));
>  	}
>  arch_res_fini:
>  	if (arch_apei_filter_addr)
> @@ -566,9 +565,9 @@ void apei_resources_release(struct apei_resources *resources)
>  	struct apei_res *res;
>  
>  	list_for_each_entry(res, &resources->iomem, list)
> -		release_mem_region(res->start, res->end - res->start);
> +		release_mem_region(res->start, resource_size(res));
>  	list_for_each_entry(res, &resources->ioport, list)
> -		release_region(res->start, res->end - res->start);
> +		release_region(res->start, resource_size(res));
>  
>  	rc = apei_resources_sub(&apei_resources_all, resources);
>  	if (rc)
> 

Re: [PATCH] ACPI, APEI, Fix use resource_size

[Borislav Petkov]

On Wed, Dec 13, 2017 at 01:19:55AM +0100, Rafael J. Wysocki wrote:
> On Monday, December 11, 2017 4:44:31 PM CET Vasyl Gomonovych wrote:
> > Use resource_size function on resource object
> > Underneath __request_region set res->end to start + n - 1
> > Lets use resourse_size to set value properly.
> > 
> > Signed-off-by: Vasyl Gomonovych <gomonovych@gmail.com>
> 
> Boris, what do you think?

So if MAINTAINERS says we review apei stuff, it doesn't mean that you
should completely stop doing that! :-P

So this looks like a fix to me since it didn't do the + 1 before.

I don't believe this would break any APEI stuff but it should be tested
a little just in case.

> > ---
> >  drivers/acpi/apei/apei-base.c | 13 ++++++-------
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/acpi/apei/apei-base.c b/drivers/acpi/apei/apei-base.c
> > index da370e1..af712a8 100644
> > --- a/drivers/acpi/apei/apei-base.c
> > +++ b/drivers/acpi/apei/apei-base.c
> > @@ -506,8 +506,7 @@ int apei_resources_request(struct apei_resources *resources,
> >  
> >  	rc = -EINVAL;
> >  	list_for_each_entry(res, &resources->iomem, list) {
> > -		r = request_mem_region(res->start, res->end - res->start,
> > -				       desc);
> > +		r = request_mem_region(res->start, resource_size(res), desc);

Jeez, this is silly: we compute resource size just so that __request_region()
can "uncompute" it back:

        res->start = start;
        res->end = start + n - 1;

Lovely.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] ACPI, APEI, Fix use resource_size

[Rafael J. Wysocki]

On Wed, Dec 13, 2017 at 4:38 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Wed, Dec 13, 2017 at 01:19:55AM +0100, Rafael J. Wysocki wrote:
>> On Monday, December 11, 2017 4:44:31 PM CET Vasyl Gomonovych wrote:
>> > Use resource_size function on resource object
>> > Underneath __request_region set res->end to start + n - 1
>> > Lets use resourse_size to set value properly.
>> >
>> > Signed-off-by: Vasyl Gomonovych <gomonovych@gmail.com>
>>
>> Boris, what do you think?
>
> So if MAINTAINERS says we review apei stuff, it doesn't mean that you
> should completely stop doing that! :-P

Which is exactly why you've got this question at all. ;-)

> So this looks like a fix to me since it didn't do the + 1 before.
>
> I don't believe this would break any APEI stuff but it should be tested
> a little just in case.

OK, let's queue it up for 4.16, then.

Re: [PATCH] ACPI, APEI, Fix use resource_size

[Borislav Petkov]

On Thu, Dec 14, 2017 at 12:07:30AM +0100, Rafael J. Wysocki wrote:
> Which is exactly why you've got this question at all. ;-)

So you're saying, you've got your minions to do review for you - you can
lay back and play all managerial now, huh?

:-P

> OK, let's queue it up for 4.16, then.

Yeah, just keep an eye on reports of mapping errors...

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] ACPI, APEI, Fix use resource_size

[Rafael J. Wysocki]

On Thu, Dec 14, 2017 at 10:37 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Thu, Dec 14, 2017 at 12:07:30AM +0100, Rafael J. Wysocki wrote:
>> Which is exactly why you've got this question at all. ;-)
>
> So you're saying, you've got your minions to do review for you - you can
> lay back and play all managerial now, huh?
>
> :-P
>
>> OK, let's queue it up for 4.16, then.
>
> Yeah, just keep an eye on reports of mapping errors...

This appears to cause sparse to complain:
drivers/acpi/apei/apei-base.c:509:21: sparse: incorrect type in
argument 1 (different base types)

It looks like that's a false-positive to me, though.

Re: [PATCH] ACPI, APEI, Fix use resource_size

[Rafael J. Wysocki]

On Sat, Dec 16, 2017 at 11:34 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Thu, Dec 14, 2017 at 10:37 AM, Borislav Petkov <bp@alien8.de> wrote:
>> On Thu, Dec 14, 2017 at 12:07:30AM +0100, Rafael J. Wysocki wrote:
>>> Which is exactly why you've got this question at all. ;-)
>>
>> So you're saying, you've got your minions to do review for you - you can
>> lay back and play all managerial now, huh?
>>
>> :-P
>>
>>> OK, let's queue it up for 4.16, then.
>>
>> Yeah, just keep an eye on reports of mapping errors...
>
> This appears to cause sparse to complain:
> drivers/acpi/apei/apei-base.c:509:21: sparse: incorrect type in
> argument 1 (different base types)
>
> It looks like that's a false-positive to me, though.

Ah, OK

It is not a false-positive.

The argument of resource_size() is (const struct resource *) and res
is of type (struct apei_res *).

I'm dropping this one.

Re: [PATCH] ACPI, APEI, Fix use resource_size

[Borislav Petkov]

On Sat, Dec 16, 2017 at 11:43:27AM +0100, Rafael J. Wysocki wrote:
> The argument of resource_size() is (const struct resource *) and res
> is of type (struct apei_res *).
> 
> I'm dropping this one.

Yeah, us reviewing this one was one big FAIL.

And I tell myself everytime - build those patches first! Srsly! People
really don't even build-test their shit before submitting. Because look
at the compiler output - it is everything but quiet. Crap didn't even
build.

Geez.

In file included from ./include/linux/acpi.h:25:0,
                 from drivers/acpi/apei/apei-base.c:32:
drivers/acpi/apei/apei-base.c: In function ‘apei_resources_request’:
drivers/acpi/apei/apei-base.c:509:52: error: passing argument 1 of ‘resource_size’ from incompatible pointer type [-Werror=incompatible-pointer-type]
   r = request_mem_region(res->start, resource_size(res), desc);
                                                    ^
./include/linux/ioport.h:223:86: note: in definition of macro ‘request_mem_region’
 #define request_mem_region(start,n,name) __request_region(&iomem_resource, (start), (n), (name), 0)
                                                                                      ^
./include/linux/ioport.h:196:31: note: expected ‘const struct resource *’ but argument is of type ‘struct apei_res *’
 static inline resource_size_t resource_size(const struct resource *res)
                               ^~~~~~~~~~~~~
drivers/acpi/apei/apei-base.c:521:48: error: passing argument 1 of ‘resource_size’ from incompatible pointer type [-Werror=incompatible-pointer-type]
   r = request_region(res->start, resource_size(res), desc);
                                                ^
./include/linux/ioport.h:220:84: note: in definition of macro ‘request_region’
 #define request_region(start,n,name)  __request_region(&ioport_resource, (start), (n), (name), 0)
                                                                                    ^
./include/linux/ioport.h:196:31: note: expected ‘const struct resource *’ but argument is of type ‘struct apei_res *’
 static inline resource_size_t resource_size(const struct resource *res)
                               ^~~~~~~~~~~~~
drivers/acpi/apei/apei-base.c:544:44: error: passing argument 1 of ‘resource_size’ from incompatible pointer type [-Werror=incompatible-pointer-type]
   release_region(res->start, resource_size(res));
                                            ^
./include/linux/ioport.h:234:78: note: in definition of macro ‘release_region’
 #define release_region(start,n) __release_region(&ioport_resource, (start), (n))
                                                                              ^
./include/linux/ioport.h:196:31: note: expected ‘const struct resource *’ but argument is of type ‘struct apei_res *’
 static inline resource_size_t resource_size(const struct resource *res)
                               ^~~~~~~~~~~~~
drivers/acpi/apei/apei-base.c:551:48: error: passing argument 1 of ‘resource_size’ from incompatible pointer type [-Werror=incompatible-pointer-type]
   release_mem_region(res->start, resource_size(res));
                                                ^
./include/linux/ioport.h:235:81: note: in definition of macro ‘release_mem_region’
 #define release_mem_region(start,n) __release_region(&iomem_resource, (start), (n))
                                                                                 ^
./include/linux/ioport.h:196:31: note: expected ‘const struct resource *’ but argument is of type ‘struct apei_res *’
 static inline resource_size_t resource_size(const struct resource *res)
                               ^~~~~~~~~~~~~
drivers/acpi/apei/apei-base.c: In function ‘apei_resources_release’:
drivers/acpi/apei/apei-base.c:568:48: error: passing argument 1 of ‘resource_size’ from incompatible pointer type [-Werror=incompatible-pointer-type]
   release_mem_region(res->start, resource_size(res));
                                                ^
./include/linux/ioport.h:235:81: note: in definition of macro ‘release_mem_region’
 #define release_mem_region(start,n) __release_region(&iomem_resource, (start), (n))
                                                                                 ^
./include/linux/ioport.h:196:31: note: expected ‘const struct resource *’ but argument is of type ‘struct apei_res *’
 static inline resource_size_t resource_size(const struct resource *res)
                               ^~~~~~~~~~~~~
drivers/acpi/apei/apei-base.c:570:44: error: passing argument 1 of ‘resource_size’ from incompatible pointer type [-Werror=incompatible-pointer-type]
   release_region(res->start, resource_size(res));
                                            ^
./include/linux/ioport.h:234:78: note: in definition of macro ‘release_region’
 #define release_region(start,n) __release_region(&ioport_resource, (start), (n))
                                                                              ^
./include/linux/ioport.h:196:31: note: expected ‘const struct resource *’ but argument is of type ‘struct apei_res *’
 static inline resource_size_t resource_size(const struct resource *res)
                               ^~~~~~~~~~~~~
cc1: some warnings being treated as errors
scripts/Makefile.build:310: recipe for target 'drivers/acpi/apei/apei-base.o' failed
make[1]: *** [drivers/acpi/apei/apei-base.o] Error 1
make[1]: *** Waiting for unfinished jobs....
Makefile:1674: recipe for target 'drivers/acpi/apei/' failed
make: *** [drivers/acpi/apei/] Error 2

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] ACPI, APEI, Fix use resource_size

[Rafael J. Wysocki]

On Saturday, December 16, 2017 12:56:15 PM CET Borislav Petkov wrote:
> On Sat, Dec 16, 2017 at 11:43:27AM +0100, Rafael J. Wysocki wrote:
> > The argument of resource_size() is (const struct resource *) and res
> > is of type (struct apei_res *).
> > 
> > I'm dropping this one.
> 
> Yeah, us reviewing this one was one big FAIL.

I don't know.

We didn't catch this issue, but that's what 0-day is for, isn't it? :-)

IMO it is not generally wrong to assume certain level of due diligence
on the part of submitters and that assumption is actually met most of the
time from my experience.

It sometimes isn't, but then the submitter lands in my "suspicious" list
and subsequent patches coming from them are subject to special treatment.

Which basically boils down to "Don't do that".

> And I tell myself everytime - build those patches first! Srsly! People
> really don't even build-test their shit before submitting. Because look
> at the compiler output - it is everything but quiet. Crap didn't even
> build.

Right, but at least we know that the current code isn't correct.

Thanks,
Rafael