* [PATCH] APEI / ERST: Fix missing error handling in erst_reader()
@ 2017-12-14 12:31 Takashi Iwai
2017-12-15 1:01 ` Rafael J. Wysocki
2017-12-17 17:37 ` Rafael J. Wysocki
0 siblings, 2 replies; 6+ messages in thread
From: Takashi Iwai @ 2017-12-14 12:31 UTC (permalink / raw)
To: Kees Cook
Cc: Anton Vorontsov, Colin Cross, Tony Luck, Rafael J . Wysocki,
Jerry Tang, linux-acpi, linux-kernel
The commit f6f828513290 ("pstore: pass allocated memory region back to
caller") changed the check of the return value from erst_read() in
erst_reader() in the following way:
if (len == -ENOENT)
goto skip;
- else if (len < 0) {
- rc = -1;
+ else if (len < sizeof(*rcd)) {
+ rc = -EIO;
goto out;
This introduced another bug: since the comparison with sizeof() is
cast to unsigned, a negative len value doesn't hit any longer.
As a result, when an error is returned from erst_read(), the code
falls through, and it may eventually lead to some weird thing like
memory corruption.
This patch adds the negative error value check more explicitly for
addressing the issue.
Fixes: f6f828513290 ("pstore: pass allocated memory region back to caller")
Cc: <stable@vger.kernel.org>
Tested-by: Jerry Tang <jtang@suse.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
drivers/acpi/apei/erst.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
index 6742f6c68034..9bff853e85f3 100644
--- a/drivers/acpi/apei/erst.c
+++ b/drivers/acpi/apei/erst.c
@@ -1007,7 +1007,7 @@ static ssize_t erst_reader(struct pstore_record *record)
/* The record may be cleared by others, try read next record */
if (len == -ENOENT)
goto skip;
- else if (len < sizeof(*rcd)) {
+ else if (len < 0 || len < sizeof(*rcd)) {
rc = -EIO;
goto out;
}
--
2.15.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] APEI / ERST: Fix missing error handling in erst_reader()
2017-12-14 12:31 [PATCH] APEI / ERST: Fix missing error handling in erst_reader() Takashi Iwai
@ 2017-12-15 1:01 ` Rafael J. Wysocki
2017-12-15 1:05 ` Kees Cook
2017-12-15 7:22 ` Takashi Iwai
2017-12-17 17:37 ` Rafael J. Wysocki
1 sibling, 2 replies; 6+ messages in thread
From: Rafael J. Wysocki @ 2017-12-15 1:01 UTC (permalink / raw)
To: Takashi Iwai
Cc: Kees Cook, Anton Vorontsov, Colin Cross, Tony Luck,
Rafael J . Wysocki, Jerry Tang, ACPI Devel Maling List,
Linux Kernel Mailing List
On Thu, Dec 14, 2017 at 1:31 PM, Takashi Iwai <tiwai@suse.de> wrote:
> The commit f6f828513290 ("pstore: pass allocated memory region back to
> caller") changed the check of the return value from erst_read() in
> erst_reader() in the following way:
>
> if (len == -ENOENT)
> goto skip;
> - else if (len < 0) {
> - rc = -1;
> + else if (len < sizeof(*rcd)) {
> + rc = -EIO;
> goto out;
>
> This introduced another bug: since the comparison with sizeof() is
> cast to unsigned, a negative len value doesn't hit any longer.
> As a result, when an error is returned from erst_read(), the code
> falls through, and it may eventually lead to some weird thing like
> memory corruption.
>
> This patch adds the negative error value check more explicitly for
> addressing the issue.
>
> Fixes: f6f828513290 ("pstore: pass allocated memory region back to caller")
That's ancient. :-)
> Cc: <stable@vger.kernel.org>
> Tested-by: Jerry Tang <jtang@suse.com>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
> drivers/acpi/apei/erst.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
> index 6742f6c68034..9bff853e85f3 100644
> --- a/drivers/acpi/apei/erst.c
> +++ b/drivers/acpi/apei/erst.c
> @@ -1007,7 +1007,7 @@ static ssize_t erst_reader(struct pstore_record *record)
> /* The record may be cleared by others, try read next record */
> if (len == -ENOENT)
> goto skip;
> - else if (len < sizeof(*rcd)) {
> + else if (len < 0 || len < sizeof(*rcd)) {
> rc = -EIO;
> goto out;
> }
> --
OK, I'm going to queue this up unless I see objections from Boris or Tony.
Thanks,
Rafael
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] APEI / ERST: Fix missing error handling in erst_reader()
2017-12-15 1:01 ` Rafael J. Wysocki
@ 2017-12-15 1:05 ` Kees Cook
2017-12-15 7:22 ` Takashi Iwai
1 sibling, 0 replies; 6+ messages in thread
From: Kees Cook @ 2017-12-15 1:05 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Takashi Iwai, Anton Vorontsov, Colin Cross, Tony Luck,
Rafael J . Wysocki, Jerry Tang, ACPI Devel Maling List,
Linux Kernel Mailing List
On Thu, Dec 14, 2017 at 5:01 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Thu, Dec 14, 2017 at 1:31 PM, Takashi Iwai <tiwai@suse.de> wrote:
>> The commit f6f828513290 ("pstore: pass allocated memory region back to
>> caller") changed the check of the return value from erst_read() in
>> erst_reader() in the following way:
>>
>> if (len == -ENOENT)
>> goto skip;
>> - else if (len < 0) {
>> - rc = -1;
>> + else if (len < sizeof(*rcd)) {
>> + rc = -EIO;
>> goto out;
>>
>> This introduced another bug: since the comparison with sizeof() is
>> cast to unsigned, a negative len value doesn't hit any longer.
>> As a result, when an error is returned from erst_read(), the code
>> falls through, and it may eventually lead to some weird thing like
>> memory corruption.
>>
>> This patch adds the negative error value check more explicitly for
>> addressing the issue.
Ew, nice catch, thanks!
>>
>> Fixes: f6f828513290 ("pstore: pass allocated memory region back to caller")
>
> That's ancient. :-)
>
>> Cc: <stable@vger.kernel.org>
>> Tested-by: Jerry Tang <jtang@suse.com>
>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
>> ---
>> drivers/acpi/apei/erst.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
>> index 6742f6c68034..9bff853e85f3 100644
>> --- a/drivers/acpi/apei/erst.c
>> +++ b/drivers/acpi/apei/erst.c
>> @@ -1007,7 +1007,7 @@ static ssize_t erst_reader(struct pstore_record *record)
>> /* The record may be cleared by others, try read next record */
>> if (len == -ENOENT)
>> goto skip;
>> - else if (len < sizeof(*rcd)) {
>> + else if (len < 0 || len < sizeof(*rcd)) {
>> rc = -EIO;
>> goto out;
>> }
>> --
>
> OK, I'm going to queue this up unless I see objections from Boris or Tony.
Please do, yes.
Acked-by: Kees Cook <keescook@chromium.org>
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] APEI / ERST: Fix missing error handling in erst_reader()
2017-12-15 1:01 ` Rafael J. Wysocki
2017-12-15 1:05 ` Kees Cook
@ 2017-12-15 7:22 ` Takashi Iwai
2017-12-15 10:52 ` Borislav Petkov
1 sibling, 1 reply; 6+ messages in thread
From: Takashi Iwai @ 2017-12-15 7:22 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Kees Cook, Anton Vorontsov, Colin Cross, Tony Luck,
Rafael J . Wysocki, Jerry Tang, Borislav Petkov,
ACPI Devel Maling List, Linux Kernel Mailing List
On Fri, 15 Dec 2017 02:01:20 +0100,
Rafael J. Wysocki wrote:
>
> On Thu, Dec 14, 2017 at 1:31 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > The commit f6f828513290 ("pstore: pass allocated memory region back to
> > caller") changed the check of the return value from erst_read() in
> > erst_reader() in the following way:
> >
> > if (len == -ENOENT)
> > goto skip;
> > - else if (len < 0) {
> > - rc = -1;
> > + else if (len < sizeof(*rcd)) {
> > + rc = -EIO;
> > goto out;
> >
> > This introduced another bug: since the comparison with sizeof() is
> > cast to unsigned, a negative len value doesn't hit any longer.
> > As a result, when an error is returned from erst_read(), the code
> > falls through, and it may eventually lead to some weird thing like
> > memory corruption.
> >
> > This patch adds the negative error value check more explicitly for
> > addressing the issue.
> >
> > Fixes: f6f828513290 ("pstore: pass allocated memory region back to caller")
>
> That's ancient. :-)
>
> > Cc: <stable@vger.kernel.org>
> > Tested-by: Jerry Tang <jtang@suse.com>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> > drivers/acpi/apei/erst.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
> > index 6742f6c68034..9bff853e85f3 100644
> > --- a/drivers/acpi/apei/erst.c
> > +++ b/drivers/acpi/apei/erst.c
> > @@ -1007,7 +1007,7 @@ static ssize_t erst_reader(struct pstore_record *record)
> > /* The record may be cleared by others, try read next record */
> > if (len == -ENOENT)
> > goto skip;
> > - else if (len < sizeof(*rcd)) {
> > + else if (len < 0 || len < sizeof(*rcd)) {
> > rc = -EIO;
> > goto out;
> > }
> > --
>
> OK, I'm going to queue this up unless I see objections from Boris or Tony.
I missed Boris in Cc list, sorry. Now added.
Takashi
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] APEI / ERST: Fix missing error handling in erst_reader()
2017-12-15 7:22 ` Takashi Iwai
@ 2017-12-15 10:52 ` Borislav Petkov
0 siblings, 0 replies; 6+ messages in thread
From: Borislav Petkov @ 2017-12-15 10:52 UTC (permalink / raw)
To: Takashi Iwai
Cc: Rafael J. Wysocki, Kees Cook, Anton Vorontsov, Colin Cross,
Tony Luck, Rafael J . Wysocki, Jerry Tang,
ACPI Devel Maling List, Linux Kernel Mailing List
On Fri, Dec 15, 2017 at 08:22:32AM +0100, Takashi Iwai wrote:
> On Fri, 15 Dec 2017 02:01:20 +0100,
> Rafael J. Wysocki wrote:
> >
> > On Thu, Dec 14, 2017 at 1:31 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > > The commit f6f828513290 ("pstore: pass allocated memory region back to
> > > caller") changed the check of the return value from erst_read() in
> > > erst_reader() in the following way:
> > >
> > > if (len == -ENOENT)
> > > goto skip;
> > > - else if (len < 0) {
> > > - rc = -1;
> > > + else if (len < sizeof(*rcd)) {
> > > + rc = -EIO;
> > > goto out;
> > >
> > > This introduced another bug: since the comparison with sizeof() is
> > > cast to unsigned, a negative len value doesn't hit any longer.
> > > As a result, when an error is returned from erst_read(), the code
> > > falls through, and it may eventually lead to some weird thing like
> > > memory corruption.
> > >
> > > This patch adds the negative error value check more explicitly for
> > > addressing the issue.
> > >
> > > Fixes: f6f828513290 ("pstore: pass allocated memory region back to caller")
> >
> > That's ancient. :-)
> >
> > > Cc: <stable@vger.kernel.org>
> > > Tested-by: Jerry Tang <jtang@suse.com>
> > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > ---
> > > drivers/acpi/apei/erst.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
> > > index 6742f6c68034..9bff853e85f3 100644
> > > --- a/drivers/acpi/apei/erst.c
> > > +++ b/drivers/acpi/apei/erst.c
> > > @@ -1007,7 +1007,7 @@ static ssize_t erst_reader(struct pstore_record *record)
> > > /* The record may be cleared by others, try read next record */
> > > if (len == -ENOENT)
> > > goto skip;
> > > - else if (len < sizeof(*rcd)) {
> > > + else if (len < 0 || len < sizeof(*rcd)) {
> > > rc = -EIO;
> > > goto out;
> > > }
> > > --
> >
> > OK, I'm going to queue this up unless I see objections from Boris or Tony.
>
> I missed Boris in Cc list, sorry. Now added.
LGTM.
Reviewed-by: Borislav Petkov <bp@suse.de>
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] APEI / ERST: Fix missing error handling in erst_reader()
2017-12-14 12:31 [PATCH] APEI / ERST: Fix missing error handling in erst_reader() Takashi Iwai
2017-12-15 1:01 ` Rafael J. Wysocki
@ 2017-12-17 17:37 ` Rafael J. Wysocki
1 sibling, 0 replies; 6+ messages in thread
From: Rafael J. Wysocki @ 2017-12-17 17:37 UTC (permalink / raw)
To: Takashi Iwai
Cc: Kees Cook, Anton Vorontsov, Colin Cross, Tony Luck, Jerry Tang,
linux-acpi, linux-kernel
On Thursday, December 14, 2017 1:31:16 PM CET Takashi Iwai wrote:
> The commit f6f828513290 ("pstore: pass allocated memory region back to
> caller") changed the check of the return value from erst_read() in
> erst_reader() in the following way:
>
> if (len == -ENOENT)
> goto skip;
> - else if (len < 0) {
> - rc = -1;
> + else if (len < sizeof(*rcd)) {
> + rc = -EIO;
> goto out;
>
> This introduced another bug: since the comparison with sizeof() is
> cast to unsigned, a negative len value doesn't hit any longer.
> As a result, when an error is returned from erst_read(), the code
> falls through, and it may eventually lead to some weird thing like
> memory corruption.
>
> This patch adds the negative error value check more explicitly for
> addressing the issue.
>
> Fixes: f6f828513290 ("pstore: pass allocated memory region back to caller")
> Cc: <stable@vger.kernel.org>
> Tested-by: Jerry Tang <jtang@suse.com>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
> drivers/acpi/apei/erst.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
> index 6742f6c68034..9bff853e85f3 100644
> --- a/drivers/acpi/apei/erst.c
> +++ b/drivers/acpi/apei/erst.c
> @@ -1007,7 +1007,7 @@ static ssize_t erst_reader(struct pstore_record *record)
> /* The record may be cleared by others, try read next record */
> if (len == -ENOENT)
> goto skip;
> - else if (len < sizeof(*rcd)) {
> + else if (len < 0 || len < sizeof(*rcd)) {
> rc = -EIO;
> goto out;
> }
>
Applied, thanks!
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-12-17 17:38 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-14 12:31 [PATCH] APEI / ERST: Fix missing error handling in erst_reader() Takashi Iwai
2017-12-15 1:01 ` Rafael J. Wysocki
2017-12-15 1:05 ` Kees Cook
2017-12-15 7:22 ` Takashi Iwai
2017-12-15 10:52 ` Borislav Petkov
2017-12-17 17:37 ` Rafael J. Wysocki
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.