Patchwork [097/102] efivars: explicitly calculate length of VariableName

login
register
mail settings
Submitter Luis Henriques
Date April 8, 2013, 9:50 a.m.
Message ID <1365414657-29191-98-git-send-email-luis.henriques@canonical.com>
Download mbox | patch
Permalink /patch/235017/
State New
Headers show

Comments

Luis Henriques - April 8, 2013, 9:50 a.m.
3.5.7.10 -stable review patch.  If anyone has any objections, please let me know.

------------------

From: Matt Fleming <matt.fleming@intel.com>

commit ec50bd32f1672d38ddce10fb1841cbfda89cfe9a upstream.

It's not wise to assume VariableNameSize represents the length of
VariableName, as not all firmware updates VariableNameSize in the same
way (some don't update it at all if EFI_SUCCESS is returned). There
are even implementations out there that update VariableNameSize with
values that are both larger than the string returned in VariableName
and smaller than the buffer passed to GetNextVariableName(), which
resulted in the following bug report from Michael Schroeder,

  > On HP z220 system (firmware version 1.54), some EFI variables are
  > incorrectly named :
  >
  > ls -d /sys/firmware/efi/vars/*8be4d* | grep -v -- -8be returns
  > /sys/firmware/efi/vars/dbxDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
  > /sys/firmware/efi/vars/KEKDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
  > /sys/firmware/efi/vars/SecureBoot-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
  > /sys/firmware/efi/vars/SetupMode-Information8be4df61-93ca-11d2-aa0d-00e098032b8c

The issue here is that because we blindly use VariableNameSize without
verifying its value, we can potentially read garbage values from the
buffer containing VariableName if VariableNameSize is larger than the
length of VariableName.

Since VariableName is a string, we can calculate its size by searching
for the terminating NULL character.

Reported-by: Frederic Crozat <fcrozat@suse.com>
Cc: Matthew Garrett <mjg59@srcf.ucam.org>
Cc: Josh Boyer <jwboyer@redhat.com>
Cc: Michael Schroeder <mls@suse.com>
Cc: Lee, Chun-Yi <jlee@suse.com>
Cc: Lingzhu Xiang <lxiang@redhat.com>
Cc: Seiji Aguchi <seiji.aguchi@hds.com>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
[ Backported for 3.4-stable. Removed workqueue code added in a93bc0c 3.9-rc1. ]
Signed-off-by: Lingzhu Xiang <lxiang@redhat.com>
Reviewed-by: CAI Qian <caiqian@redhat.com>
Signed-off-by: Luis Henriques <luis.henriques@canonical.com>
---
 drivers/firmware/efivars.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)
Ben Hutchings - April 9, 2013, 10:45 p.m.
On Mon, 2013-04-08 at 10:50 +0100, Luis Henriques wrote:
> 3.5.7.10 -stable review patch.  If anyone has any objections, please let me know.
> 
> ------------------
> 
> From: Matt Fleming <matt.fleming@intel.com>
> 
> commit ec50bd32f1672d38ddce10fb1841cbfda89cfe9a upstream.
> 
> It's not wise to assume VariableNameSize represents the length of
> VariableName, as not all firmware updates VariableNameSize in the same
> way (some don't update it at all if EFI_SUCCESS is returned). There
> are even implementations out there that update VariableNameSize with
> values that are both larger than the string returned in VariableName
> and smaller than the buffer passed to GetNextVariableName(), which
> resulted in the following bug report from Michael Schroeder,
> 
>   > On HP z220 system (firmware version 1.54), some EFI variables are
>   > incorrectly named :
>   >
>   > ls -d /sys/firmware/efi/vars/*8be4d* | grep -v -- -8be returns
>   > /sys/firmware/efi/vars/dbxDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
>   > /sys/firmware/efi/vars/KEKDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
>   > /sys/firmware/efi/vars/SecureBoot-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
>   > /sys/firmware/efi/vars/SetupMode-Information8be4df61-93ca-11d2-aa0d-00e098032b8c
> 
> The issue here is that because we blindly use VariableNameSize without
> verifying its value, we can potentially read garbage values from the
> buffer containing VariableName if VariableNameSize is larger than the
> length of VariableName.
> 
> Since VariableName is a string, we can calculate its size by searching
> for the terminating NULL character.
> 
> Reported-by: Frederic Crozat <fcrozat@suse.com>
> Cc: Matthew Garrett <mjg59@srcf.ucam.org>
> Cc: Josh Boyer <jwboyer@redhat.com>
> Cc: Michael Schroeder <mls@suse.com>
> Cc: Lee, Chun-Yi <jlee@suse.com>
> Cc: Lingzhu Xiang <lxiang@redhat.com>
> Cc: Seiji Aguchi <seiji.aguchi@hds.com>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> [ Backported for 3.4-stable. Removed workqueue code added in a93bc0c 3.9-rc1. ]
[...]

I thought the workqueue addition was a worthwhile fix in its own right,
so for 3.2.y I cherry-picked that as well.

Ben.
Luis Henriques - April 10, 2013, 9:35 a.m.
On Tue, Apr 09, 2013 at 11:45:06PM +0100, Ben Hutchings wrote:
> On Mon, 2013-04-08 at 10:50 +0100, Luis Henriques wrote:
> > 3.5.7.10 -stable review patch.  If anyone has any objections, please let me know.
> > 
> > ------------------
> > 
> > From: Matt Fleming <matt.fleming@intel.com>
> > 
> > commit ec50bd32f1672d38ddce10fb1841cbfda89cfe9a upstream.
> > 
> > It's not wise to assume VariableNameSize represents the length of
> > VariableName, as not all firmware updates VariableNameSize in the same
> > way (some don't update it at all if EFI_SUCCESS is returned). There
> > are even implementations out there that update VariableNameSize with
> > values that are both larger than the string returned in VariableName
> > and smaller than the buffer passed to GetNextVariableName(), which
> > resulted in the following bug report from Michael Schroeder,
> > 
> >   > On HP z220 system (firmware version 1.54), some EFI variables are
> >   > incorrectly named :
> >   >
> >   > ls -d /sys/firmware/efi/vars/*8be4d* | grep -v -- -8be returns
> >   > /sys/firmware/efi/vars/dbxDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> >   > /sys/firmware/efi/vars/KEKDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> >   > /sys/firmware/efi/vars/SecureBoot-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> >   > /sys/firmware/efi/vars/SetupMode-Information8be4df61-93ca-11d2-aa0d-00e098032b8c
> > 
> > The issue here is that because we blindly use VariableNameSize without
> > verifying its value, we can potentially read garbage values from the
> > buffer containing VariableName if VariableNameSize is larger than the
> > length of VariableName.
> > 
> > Since VariableName is a string, we can calculate its size by searching
> > for the terminating NULL character.
> > 
> > Reported-by: Frederic Crozat <fcrozat@suse.com>
> > Cc: Matthew Garrett <mjg59@srcf.ucam.org>
> > Cc: Josh Boyer <jwboyer@redhat.com>
> > Cc: Michael Schroeder <mls@suse.com>
> > Cc: Lee, Chun-Yi <jlee@suse.com>
> > Cc: Lingzhu Xiang <lxiang@redhat.com>
> > Cc: Seiji Aguchi <seiji.aguchi@hds.com>
> > Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> > [ Backported for 3.4-stable. Removed workqueue code added in a93bc0c 3.9-rc1. ]
> [...]
> 
> I thought the workqueue addition was a worthwhile fix in its own right,
> so for 3.2.y I cherry-picked that as well.

Ok, makes sense to me too.  So here's what I'll do: for this release
I'll just drop this commit and e971318bbed610e28bb3fde9d548e6aaf0a6b02e
('efivars: Handle duplicate names from get_next_variable()').

I'll queue them again for next release along with commit
a93bc0c6e07ed9bac44700280e65e2945d864fd4 ('efi_pstore: Introducing
workqueue updating sysfs').

Thanks for your review Ben.

Cheers,
--
Luis
Lingzhu Xiang - April 10, 2013, 10:27 a.m.
On 04/10/2013 06:45 AM, Ben Hutchings wrote:
> On Mon, 2013-04-08 at 10:50 +0100, Luis Henriques wrote:
>> 3.5.7.10 -stable review patch.  If anyone has any objections, please let me know.
>>
>> ------------------
>>
>> From: Matt Fleming <matt.fleming@intel.com>
>>
>> commit ec50bd32f1672d38ddce10fb1841cbfda89cfe9a upstream.
>>
>> It's not wise to assume VariableNameSize represents the length of
>> VariableName, as not all firmware updates VariableNameSize in the same
>> way (some don't update it at all if EFI_SUCCESS is returned). There
>> are even implementations out there that update VariableNameSize with
>> values that are both larger than the string returned in VariableName
>> and smaller than the buffer passed to GetNextVariableName(), which
>> resulted in the following bug report from Michael Schroeder,
>>
>>    > On HP z220 system (firmware version 1.54), some EFI variables are
>>    > incorrectly named :
>>    >
>>    > ls -d /sys/firmware/efi/vars/*8be4d* | grep -v -- -8be returns
>>    > /sys/firmware/efi/vars/dbxDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
>>    > /sys/firmware/efi/vars/KEKDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
>>    > /sys/firmware/efi/vars/SecureBoot-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
>>    > /sys/firmware/efi/vars/SetupMode-Information8be4df61-93ca-11d2-aa0d-00e098032b8c
>>
>> The issue here is that because we blindly use VariableNameSize without
>> verifying its value, we can potentially read garbage values from the
>> buffer containing VariableName if VariableNameSize is larger than the
>> length of VariableName.
>>
>> Since VariableName is a string, we can calculate its size by searching
>> for the terminating NULL character.
>>
>> Reported-by: Frederic Crozat <fcrozat@suse.com>
>> Cc: Matthew Garrett <mjg59@srcf.ucam.org>
>> Cc: Josh Boyer <jwboyer@redhat.com>
>> Cc: Michael Schroeder <mls@suse.com>
>> Cc: Lee, Chun-Yi <jlee@suse.com>
>> Cc: Lingzhu Xiang <lxiang@redhat.com>
>> Cc: Seiji Aguchi <seiji.aguchi@hds.com>
>> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
>> [ Backported for 3.4-stable. Removed workqueue code added in a93bc0c 3.9-rc1. ]
> [...]
>
> I thought the workqueue addition was a worthwhile fix in its own right,
> so for 3.2.y I cherry-picked that as well.

FWIW, the workqueue patch is 1/2 of this patchset[1] fixing closely 
related problems. The other one is 81fa4e58.

[1]: http://article.gmane.org/gmane.linux.kernel/1439570

I tried to avoid pulling too much for stable because the patchset is 
quite large and I suspect the problem it fixes is only theoretical. I 
reported the original bug but was unable to break anything except 
getting call traces with various CONFIG_DEBUG_*.

What's your opinion, Seiji?


Lingzhu Xiang
Luis Henriques - April 10, 2013, 12:17 p.m.
On Wed, Apr 10, 2013 at 06:27:13PM +0800, Lingzhu Xiang wrote:
> On 04/10/2013 06:45 AM, Ben Hutchings wrote:
> >On Mon, 2013-04-08 at 10:50 +0100, Luis Henriques wrote:
> >>3.5.7.10 -stable review patch.  If anyone has any objections, please let me know.
> >>
> >>------------------
> >>
> >>From: Matt Fleming <matt.fleming@intel.com>
> >>
> >>commit ec50bd32f1672d38ddce10fb1841cbfda89cfe9a upstream.
> >>
> >>It's not wise to assume VariableNameSize represents the length of
> >>VariableName, as not all firmware updates VariableNameSize in the same
> >>way (some don't update it at all if EFI_SUCCESS is returned). There
> >>are even implementations out there that update VariableNameSize with
> >>values that are both larger than the string returned in VariableName
> >>and smaller than the buffer passed to GetNextVariableName(), which
> >>resulted in the following bug report from Michael Schroeder,
> >>
> >>   > On HP z220 system (firmware version 1.54), some EFI variables are
> >>   > incorrectly named :
> >>   >
> >>   > ls -d /sys/firmware/efi/vars/*8be4d* | grep -v -- -8be returns
> >>   > /sys/firmware/efi/vars/dbxDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> >>   > /sys/firmware/efi/vars/KEKDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> >>   > /sys/firmware/efi/vars/SecureBoot-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> >>   > /sys/firmware/efi/vars/SetupMode-Information8be4df61-93ca-11d2-aa0d-00e098032b8c
> >>
> >>The issue here is that because we blindly use VariableNameSize without
> >>verifying its value, we can potentially read garbage values from the
> >>buffer containing VariableName if VariableNameSize is larger than the
> >>length of VariableName.
> >>
> >>Since VariableName is a string, we can calculate its size by searching
> >>for the terminating NULL character.
> >>
> >>Reported-by: Frederic Crozat <fcrozat@suse.com>
> >>Cc: Matthew Garrett <mjg59@srcf.ucam.org>
> >>Cc: Josh Boyer <jwboyer@redhat.com>
> >>Cc: Michael Schroeder <mls@suse.com>
> >>Cc: Lee, Chun-Yi <jlee@suse.com>
> >>Cc: Lingzhu Xiang <lxiang@redhat.com>
> >>Cc: Seiji Aguchi <seiji.aguchi@hds.com>
> >>Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> >>[ Backported for 3.4-stable. Removed workqueue code added in a93bc0c 3.9-rc1. ]
> >[...]
> >
> >I thought the workqueue addition was a worthwhile fix in its own right,
> >so for 3.2.y I cherry-picked that as well.
> 
> FWIW, the workqueue patch is 1/2 of this patchset[1] fixing closely
> related problems. The other one is 81fa4e58.
> 
> [1]: http://article.gmane.org/gmane.linux.kernel/1439570
> 
> I tried to avoid pulling too much for stable because the patchset is
> quite large and I suspect the problem it fixes is only theoretical.
> I reported the original bug but was unable to break anything except
> getting call traces with various CONFIG_DEBUG_*.
> 
> What's your opinion, Seiji?

Ok, so just to clarify: you're suggesting me to pick the following commits:

81fa4e581d9283f7992a0d8c534bb141eb840a14 efivars: Disable external interrupt while holding efivars->lock
a93bc0c6e07ed9bac44700280e65e2945d864fd4 efi_pstore: Introducing workqueue updating sysfs
ec50bd32f1672d38ddce10fb1841cbfda89cfe9a efivars: explicitly calculate length of VariableName
e971318bbed610e28bb3fde9d548e6aaf0a6b02e efivars: Handle duplicate names from get_next_variable()

Is this correct?

Cheers,
--
Luis
Seiji Aguchi - April 10, 2013, 3:57 p.m.
> -----Original Message-----
> From: Luis Henriques [mailto:luis.henriques@canonical.com]
> Sent: Wednesday, April 10, 2013 8:18 AM
> To: Lingzhu Xiang
> Cc: Ben Hutchings; Seiji Aguchi; linux-kernel@vger.kernel.org; stable@vger.kernel.org; kernel-team@lists.ubuntu.com; Matthew
> Garrett; Josh Boyer; Michael Schroeder; Lee, Chun-Yi; Matt Fleming
> Subject: Re: [PATCH 097/102] efivars: explicitly calculate length of VariableName
> 
> On Wed, Apr 10, 2013 at 06:27:13PM +0800, Lingzhu Xiang wrote:
> > On 04/10/2013 06:45 AM, Ben Hutchings wrote:
> > >On Mon, 2013-04-08 at 10:50 +0100, Luis Henriques wrote:
> > >>3.5.7.10 -stable review patch.  If anyone has any objections, please let me know.
> > >>
> > >>------------------
> > >>
> > >>From: Matt Fleming <matt.fleming@intel.com>
> > >>
> > >>commit ec50bd32f1672d38ddce10fb1841cbfda89cfe9a upstream.
> > >>
> > >>It's not wise to assume VariableNameSize represents the length of
> > >>VariableName, as not all firmware updates VariableNameSize in the
> > >>same way (some don't update it at all if EFI_SUCCESS is returned).
> > >>There are even implementations out there that update
> > >>VariableNameSize with values that are both larger than the string
> > >>returned in VariableName and smaller than the buffer passed to
> > >>GetNextVariableName(), which resulted in the following bug report
> > >>from Michael Schroeder,
> > >>
> > >>   > On HP z220 system (firmware version 1.54), some EFI variables are
> > >>   > incorrectly named :
> > >>   >
> > >>   > ls -d /sys/firmware/efi/vars/*8be4d* | grep -v -- -8be returns
> > >>   > /sys/firmware/efi/vars/dbxDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> > >>   > /sys/firmware/efi/vars/KEKDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> > >>   > /sys/firmware/efi/vars/SecureBoot-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> > >>   >
> > >> /sys/firmware/efi/vars/SetupMode-Information8be4df61-93ca-11d2-aa0d
> > >> -00e098032b8c
> > >>
> > >>The issue here is that because we blindly use VariableNameSize
> > >>without verifying its value, we can potentially read garbage values
> > >>from the buffer containing VariableName if VariableNameSize is
> > >>larger than the length of VariableName.
> > >>
> > >>Since VariableName is a string, we can calculate its size by
> > >>searching for the terminating NULL character.
> > >>
> > >>Reported-by: Frederic Crozat <fcrozat@suse.com>
> > >>Cc: Matthew Garrett <mjg59@srcf.ucam.org>
> > >>Cc: Josh Boyer <jwboyer@redhat.com>
> > >>Cc: Michael Schroeder <mls@suse.com>
> > >>Cc: Lee, Chun-Yi <jlee@suse.com>
> > >>Cc: Lingzhu Xiang <lxiang@redhat.com>
> > >>Cc: Seiji Aguchi <seiji.aguchi@hds.com>
> > >>Signed-off-by: Matt Fleming <matt.fleming@intel.com> [ Backported
> > >>for 3.4-stable. Removed workqueue code added in a93bc0c 3.9-rc1. ]
> > >[...]
> > >
> > >I thought the workqueue addition was a worthwhile fix in its own
> > >right, so for 3.2.y I cherry-picked that as well.
> >
> > FWIW, the workqueue patch is 1/2 of this patchset[1] fixing closely
> > related problems. The other one is 81fa4e58.
> >
> > [1]: http://article.gmane.org/gmane.linux.kernel/1439570
> >
> > I tried to avoid pulling too much for stable because the patchset is
> > quite large and I suspect the problem it fixes is only theoretical.
> > I reported the original bug but was unable to break anything except
> > getting call traces with various CONFIG_DEBUG_*.
> >
> > What's your opinion, Seiji?
> 
> Ok, so just to clarify: you're suggesting me to pick the following commits:
> 
> 81fa4e581d9283f7992a0d8c534bb141eb840a14 efivars: Disable external interrupt while holding efivars->lock
> a93bc0c6e07ed9bac44700280e65e2945d864fd4 efi_pstore: Introducing workqueue updating sysfs
> ec50bd32f1672d38ddce10fb1841cbfda89cfe9a efivars: explicitly calculate length of VariableName
> e971318bbed610e28bb3fde9d548e6aaf0a6b02e efivars: Handle duplicate names from get_next_variable()

I agree to add these commits to a stable tree.

Seiji

> 
> Is this correct?
> 
> Cheers,
> --
> Luis
Luis Henriques - April 11, 2013, 9:12 a.m.
On Wed, Apr 10, 2013 at 03:57:12PM +0000, Seiji Aguchi wrote:
> 
> 
> > -----Original Message-----
> > From: Luis Henriques [mailto:luis.henriques@canonical.com]
> > Sent: Wednesday, April 10, 2013 8:18 AM
> > To: Lingzhu Xiang
> > Cc: Ben Hutchings; Seiji Aguchi; linux-kernel@vger.kernel.org; stable@vger.kernel.org; kernel-team@lists.ubuntu.com; Matthew
> > Garrett; Josh Boyer; Michael Schroeder; Lee, Chun-Yi; Matt Fleming
> > Subject: Re: [PATCH 097/102] efivars: explicitly calculate length of VariableName
> > 
> > On Wed, Apr 10, 2013 at 06:27:13PM +0800, Lingzhu Xiang wrote:
> > > On 04/10/2013 06:45 AM, Ben Hutchings wrote:
> > > >On Mon, 2013-04-08 at 10:50 +0100, Luis Henriques wrote:
> > > >>3.5.7.10 -stable review patch.  If anyone has any objections, please let me know.
> > > >>
> > > >>------------------
> > > >>
> > > >>From: Matt Fleming <matt.fleming@intel.com>
> > > >>
> > > >>commit ec50bd32f1672d38ddce10fb1841cbfda89cfe9a upstream.
> > > >>
> > > >>It's not wise to assume VariableNameSize represents the length of
> > > >>VariableName, as not all firmware updates VariableNameSize in the
> > > >>same way (some don't update it at all if EFI_SUCCESS is returned).
> > > >>There are even implementations out there that update
> > > >>VariableNameSize with values that are both larger than the string
> > > >>returned in VariableName and smaller than the buffer passed to
> > > >>GetNextVariableName(), which resulted in the following bug report
> > > >>from Michael Schroeder,
> > > >>
> > > >>   > On HP z220 system (firmware version 1.54), some EFI variables are
> > > >>   > incorrectly named :
> > > >>   >
> > > >>   > ls -d /sys/firmware/efi/vars/*8be4d* | grep -v -- -8be returns
> > > >>   > /sys/firmware/efi/vars/dbxDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> > > >>   > /sys/firmware/efi/vars/KEKDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> > > >>   > /sys/firmware/efi/vars/SecureBoot-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> > > >>   >
> > > >> /sys/firmware/efi/vars/SetupMode-Information8be4df61-93ca-11d2-aa0d
> > > >> -00e098032b8c
> > > >>
> > > >>The issue here is that because we blindly use VariableNameSize
> > > >>without verifying its value, we can potentially read garbage values
> > > >>from the buffer containing VariableName if VariableNameSize is
> > > >>larger than the length of VariableName.
> > > >>
> > > >>Since VariableName is a string, we can calculate its size by
> > > >>searching for the terminating NULL character.
> > > >>
> > > >>Reported-by: Frederic Crozat <fcrozat@suse.com>
> > > >>Cc: Matthew Garrett <mjg59@srcf.ucam.org>
> > > >>Cc: Josh Boyer <jwboyer@redhat.com>
> > > >>Cc: Michael Schroeder <mls@suse.com>
> > > >>Cc: Lee, Chun-Yi <jlee@suse.com>
> > > >>Cc: Lingzhu Xiang <lxiang@redhat.com>
> > > >>Cc: Seiji Aguchi <seiji.aguchi@hds.com>
> > > >>Signed-off-by: Matt Fleming <matt.fleming@intel.com> [ Backported
> > > >>for 3.4-stable. Removed workqueue code added in a93bc0c 3.9-rc1. ]
> > > >[...]
> > > >
> > > >I thought the workqueue addition was a worthwhile fix in its own
> > > >right, so for 3.2.y I cherry-picked that as well.
> > >
> > > FWIW, the workqueue patch is 1/2 of this patchset[1] fixing closely
> > > related problems. The other one is 81fa4e58.
> > >
> > > [1]: http://article.gmane.org/gmane.linux.kernel/1439570
> > >
> > > I tried to avoid pulling too much for stable because the patchset is
> > > quite large and I suspect the problem it fixes is only theoretical.
> > > I reported the original bug but was unable to break anything except
> > > getting call traces with various CONFIG_DEBUG_*.
> > >
> > > What's your opinion, Seiji?
> > 
> > Ok, so just to clarify: you're suggesting me to pick the following commits:
> > 
> > 81fa4e581d9283f7992a0d8c534bb141eb840a14 efivars: Disable external interrupt while holding efivars->lock
> > a93bc0c6e07ed9bac44700280e65e2945d864fd4 efi_pstore: Introducing workqueue updating sysfs
> > ec50bd32f1672d38ddce10fb1841cbfda89cfe9a efivars: explicitly calculate length of VariableName
> > e971318bbed610e28bb3fde9d548e6aaf0a6b02e efivars: Handle duplicate names from get_next_variable()
> 
> I agree to add these commits to a stable tree.
> 
Thank you Seiji.  I'll queue these for the next 3.5 kernel.

Cheers,
--
Luis
Luis Henriques - April 16, 2013, 10:33 a.m.
(Adding Ben back to the CC list; not sure how his email was dropped)

On Thu, Apr 11, 2013 at 10:12:56AM +0100, Luis Henriques wrote:
> On Wed, Apr 10, 2013 at 03:57:12PM +0000, Seiji Aguchi wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Luis Henriques [mailto:luis.henriques@canonical.com]
> > > Sent: Wednesday, April 10, 2013 8:18 AM
> > > To: Lingzhu Xiang
> > > Cc: Ben Hutchings; Seiji Aguchi; linux-kernel@vger.kernel.org; stable@vger.kernel.org; kernel-team@lists.ubuntu.com; Matthew
> > > Garrett; Josh Boyer; Michael Schroeder; Lee, Chun-Yi; Matt Fleming
> > > Subject: Re: [PATCH 097/102] efivars: explicitly calculate length of VariableName
> > > 
> > > On Wed, Apr 10, 2013 at 06:27:13PM +0800, Lingzhu Xiang wrote:
> > > > On 04/10/2013 06:45 AM, Ben Hutchings wrote:
> > > > >On Mon, 2013-04-08 at 10:50 +0100, Luis Henriques wrote:
> > > > >>3.5.7.10 -stable review patch.  If anyone has any objections, please let me know.
> > > > >>
> > > > >>------------------
> > > > >>
> > > > >>From: Matt Fleming <matt.fleming@intel.com>
> > > > >>
> > > > >>commit ec50bd32f1672d38ddce10fb1841cbfda89cfe9a upstream.
> > > > >>
> > > > >>It's not wise to assume VariableNameSize represents the length of
> > > > >>VariableName, as not all firmware updates VariableNameSize in the
> > > > >>same way (some don't update it at all if EFI_SUCCESS is returned).
> > > > >>There are even implementations out there that update
> > > > >>VariableNameSize with values that are both larger than the string
> > > > >>returned in VariableName and smaller than the buffer passed to
> > > > >>GetNextVariableName(), which resulted in the following bug report
> > > > >>from Michael Schroeder,
> > > > >>
> > > > >>   > On HP z220 system (firmware version 1.54), some EFI variables are
> > > > >>   > incorrectly named :
> > > > >>   >
> > > > >>   > ls -d /sys/firmware/efi/vars/*8be4d* | grep -v -- -8be returns
> > > > >>   > /sys/firmware/efi/vars/dbxDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> > > > >>   > /sys/firmware/efi/vars/KEKDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> > > > >>   > /sys/firmware/efi/vars/SecureBoot-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> > > > >>   >
> > > > >> /sys/firmware/efi/vars/SetupMode-Information8be4df61-93ca-11d2-aa0d
> > > > >> -00e098032b8c
> > > > >>
> > > > >>The issue here is that because we blindly use VariableNameSize
> > > > >>without verifying its value, we can potentially read garbage values
> > > > >>from the buffer containing VariableName if VariableNameSize is
> > > > >>larger than the length of VariableName.
> > > > >>
> > > > >>Since VariableName is a string, we can calculate its size by
> > > > >>searching for the terminating NULL character.
> > > > >>
> > > > >>Reported-by: Frederic Crozat <fcrozat@suse.com>
> > > > >>Cc: Matthew Garrett <mjg59@srcf.ucam.org>
> > > > >>Cc: Josh Boyer <jwboyer@redhat.com>
> > > > >>Cc: Michael Schroeder <mls@suse.com>
> > > > >>Cc: Lee, Chun-Yi <jlee@suse.com>
> > > > >>Cc: Lingzhu Xiang <lxiang@redhat.com>
> > > > >>Cc: Seiji Aguchi <seiji.aguchi@hds.com>
> > > > >>Signed-off-by: Matt Fleming <matt.fleming@intel.com> [ Backported
> > > > >>for 3.4-stable. Removed workqueue code added in a93bc0c 3.9-rc1. ]
> > > > >[...]
> > > > >
> > > > >I thought the workqueue addition was a worthwhile fix in its own
> > > > >right, so for 3.2.y I cherry-picked that as well.
> > > >
> > > > FWIW, the workqueue patch is 1/2 of this patchset[1] fixing closely
> > > > related problems. The other one is 81fa4e58.
> > > >
> > > > [1]: http://article.gmane.org/gmane.linux.kernel/1439570
> > > >
> > > > I tried to avoid pulling too much for stable because the patchset is
> > > > quite large and I suspect the problem it fixes is only theoretical.
> > > > I reported the original bug but was unable to break anything except
> > > > getting call traces with various CONFIG_DEBUG_*.
> > > >
> > > > What's your opinion, Seiji?
> > > 
> > > Ok, so just to clarify: you're suggesting me to pick the following commits:
> > > 
> > > 81fa4e581d9283f7992a0d8c534bb141eb840a14 efivars: Disable external interrupt while holding efivars->lock
> > > a93bc0c6e07ed9bac44700280e65e2945d864fd4 efi_pstore: Introducing workqueue updating sysfs
> > > ec50bd32f1672d38ddce10fb1841cbfda89cfe9a efivars: explicitly calculate length of VariableName
> > > e971318bbed610e28bb3fde9d548e6aaf0a6b02e efivars: Handle duplicate names from get_next_variable()
> > 
> > I agree to add these commits to a stable tree.
> > 
> Thank you Seiji.  I'll queue these for the next 3.5 kernel.

So, after spending some time around this, I ended up picking much
more commits than those listed above.  Since this code in 3.5.y wasn't
that different from Ben's 3.2.y, I took the liberty of picking all the
bits I was missing from there.  Here's what I ended up with:

d80a361d779a9f19498943d1ca84243209cd5647 efi_pstore: Check remaining space with QueryVariableInfo() before writing data
81fa4e581d9283f7992a0d8c534bb141eb840a14 efivars: Disable external interrupt while holding efivars->lock
68d929862e29a8b52a7f2f2f86a0600423b093cd efi: be more paranoid about available space when creating variables
ed9dc8ce7a1c8115dba9483a9b51df8b63a2e0ef efivars: Allow disabling use as a pstore backend
ec0971ba5372a4dfa753f232449d23a8fd98490e efivars: Add module parameter to disable use as a pstore backend
ca0ba26fbbd2d81c43085df49ce0abfe34535a90 efivars: Fix check for CONFIG_EFI_VARS_PSTORE_DEFAULT_DISABLE
a93bc0c6e07ed9bac44700280e65e2945d864fd4 efi_pstore: Introducing workqueue updating sysfs
ec50bd32f1672d38ddce10fb1841cbfda89cfe9a efivars: explicitly calculate length of VariableName
e971318bbed610e28bb3fde9d548e6aaf0a6b02e efivars: Handle duplicate names from get_next_variable()
                                         efivars: pstore: Do not check size when erasing variable

(this last commit was picked from 3.2.y directly, its not an upstream
commit)

I'll be sending out 3.5.7.11 for review later this week, and I would
really appreciate any feedback on these commits.

(Ben: Thanks a lot, since most of the backport work was took from your
tree.)

Cheers,
--
Luis
Lingzhu Xiang - April 17, 2013, 4:37 a.m.
On 04/16/2013 06:33 PM, Luis Henriques wrote:
> 68d929862e29a8b52a7f2f2f86a0600423b093cd efi: be more paranoid about available space when creating variables

This prevents a bricking issue for some Samsung devices but causes 
regression on many other machines.

https://bugzilla.kernel.org/show_bug.cgi?id=55471
https://bugzilla.redhat.com/show_bug.cgi?id=947142
http://article.gmane.org/gmane.linux.kernel.efi/1078
http://article.gmane.org/gmane.linux.kernel.stable/47150

This patchset will fix it again:

http://thread.gmane.org/gmane.linux.kernel.efi/1081
Andy Whitcroft - April 17, 2013, 11:56 a.m.
On Wed, Apr 17, 2013 at 12:37:43PM +0800, Lingzhu Xiang wrote:
> On 04/16/2013 06:33 PM, Luis Henriques wrote:
> >68d929862e29a8b52a7f2f2f86a0600423b093cd efi: be more paranoid about available space when creating variables
> 
> This prevents a bricking issue for some Samsung devices but causes
> regression on many other machines.
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=55471
> https://bugzilla.redhat.com/show_bug.cgi?id=947142
> http://article.gmane.org/gmane.linux.kernel.efi/1078
> http://article.gmane.org/gmane.linux.kernel.stable/47150
> 
> This patchset will fix it again:
> 
> http://thread.gmane.org/gmane.linux.kernel.efi/1081

Thanks for the pointer.  I note that in that thread you yourself imply
there are still issues after applying them?  Was that accurate, is there
yet further patches needed?

-apw
Lingzhu Xiang - April 17, 2013, 12:13 p.m.
On 04/17/2013 07:56 PM, Andy Whitcroft wrote:
> On Wed, Apr 17, 2013 at 12:37:43PM +0800, Lingzhu Xiang wrote:
>> On 04/16/2013 06:33 PM, Luis Henriques wrote:
>>> 68d929862e29a8b52a7f2f2f86a0600423b093cd efi: be more paranoid about available space when creating variables
>>
>> This prevents a bricking issue for some Samsung devices but causes
>> regression on many other machines.
>>
>> https://bugzilla.kernel.org/show_bug.cgi?id=55471
>> https://bugzilla.redhat.com/show_bug.cgi?id=947142
>> http://article.gmane.org/gmane.linux.kernel.efi/1078
>> http://article.gmane.org/gmane.linux.kernel.stable/47150
>>
>> This patchset will fix it again:
>>
>> http://thread.gmane.org/gmane.linux.kernel.efi/1081
>
> Thanks for the pointer.  I note that in that thread you yourself imply
> there are still issues after applying them?  Was that accurate, is there
> yet further patches needed?

I just find that issue. Didn't see that when writing the above.

Earlier I was testing on a different machine and the result was good. I 
guess the patch still needs some more testing.


Lingzhu
Luis Henriques - April 17, 2013, 1:28 p.m.
On Wed, Apr 17, 2013 at 08:13:57PM +0800, Lingzhu Xiang wrote:
> On 04/17/2013 07:56 PM, Andy Whitcroft wrote:
> >On Wed, Apr 17, 2013 at 12:37:43PM +0800, Lingzhu Xiang wrote:
> >>On 04/16/2013 06:33 PM, Luis Henriques wrote:
> >>>68d929862e29a8b52a7f2f2f86a0600423b093cd efi: be more paranoid about available space when creating variables
> >>
> >>This prevents a bricking issue for some Samsung devices but causes
> >>regression on many other machines.
> >>
> >>https://bugzilla.kernel.org/show_bug.cgi?id=55471
> >>https://bugzilla.redhat.com/show_bug.cgi?id=947142
> >>http://article.gmane.org/gmane.linux.kernel.efi/1078
> >>http://article.gmane.org/gmane.linux.kernel.stable/47150
> >>
> >>This patchset will fix it again:
> >>
> >>http://thread.gmane.org/gmane.linux.kernel.efi/1081
> >
> >Thanks for the pointer.  I note that in that thread you yourself imply
> >there are still issues after applying them?  Was that accurate, is there
> >yet further patches needed?
> 
> I just find that issue. Didn't see that when writing the above.
> 
> Earlier I was testing on a different machine and the result was
> good. I guess the patch still needs some more testing.

Thanks Lingzhu.

So, I guess you would recommend me to drop the whole series until we
have this patchset accepted, tested and back-ported for 3.5, correct?

(I tried to isolate the one you pointed out, but I'm afraid there are
too many dependencies between them to drop a single patch.)

Cheers,
--
Luis
Lingzhu Xiang - April 18, 2013, 3:27 a.m.
On 04/17/2013 09:28 PM, Luis Henriques wrote:
> On Wed, Apr 17, 2013 at 08:13:57PM +0800, Lingzhu Xiang wrote:
>> On 04/17/2013 07:56 PM, Andy Whitcroft wrote:
>>> On Wed, Apr 17, 2013 at 12:37:43PM +0800, Lingzhu Xiang wrote:
>>>> On 04/16/2013 06:33 PM, Luis Henriques wrote:
>>>>> 68d929862e29a8b52a7f2f2f86a0600423b093cd efi: be more paranoid about available space when creating variables
>>>>
>>>> This prevents a bricking issue for some Samsung devices but causes
>>>> regression on many other machines.
>>>>
>>>> https://bugzilla.kernel.org/show_bug.cgi?id=55471
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=947142
>>>> http://article.gmane.org/gmane.linux.kernel.efi/1078
>>>> http://article.gmane.org/gmane.linux.kernel.stable/47150
>>>>
>>>> This patchset will fix it again:
>>>>
>>>> http://thread.gmane.org/gmane.linux.kernel.efi/1081
>>>
>>> Thanks for the pointer.  I note that in that thread you yourself imply
>>> there are still issues after applying them?  Was that accurate, is there
>>> yet further patches needed?
>>
>> I just find that issue. Didn't see that when writing the above.
>>
>> Earlier I was testing on a different machine and the result was
>> good. I guess the patch still needs some more testing.
> 
> Thanks Lingzhu.
> 
> So, I guess you would recommend me to drop the whole series until we
> have this patchset accepted, tested and back-ported for 3.5, correct?
> 
> (I tried to isolate the one you pointed out, but I'm afraid there are
> too many dependencies between them to drop a single patch.)

Unfortunately yes.

The whole series are mostly solving pstore bugs and pstore trashing
firmware. If you want to prevent more bricking from happening asap,
a temporary choice is to allow disabling efi pstore altogether with these:

ed9dc8ce7a1c8115dba9483a9b51df8b63a2e0ef efivars: Allow disabling use as a pstore backend
ec0971ba5372a4dfa753f232449d23a8fd98490e efivars: Add module parameter to disable use as a pstore backend
ca0ba26fbbd2d81c43085df49ce0abfe34535a90 efivars: Fix check for CONFIG_EFI_VARS_PSTORE_DEFAULT_DISABLE
Luis Henriques - April 18, 2013, 8:58 a.m.
On Thu, Apr 18, 2013 at 11:27:48AM +0800, Lingzhu Xiang wrote:
> On 04/17/2013 09:28 PM, Luis Henriques wrote:
> > On Wed, Apr 17, 2013 at 08:13:57PM +0800, Lingzhu Xiang wrote:
> >> On 04/17/2013 07:56 PM, Andy Whitcroft wrote:
> >>> On Wed, Apr 17, 2013 at 12:37:43PM +0800, Lingzhu Xiang wrote:
> >>>> On 04/16/2013 06:33 PM, Luis Henriques wrote:
> >>>>> 68d929862e29a8b52a7f2f2f86a0600423b093cd efi: be more paranoid about available space when creating variables
> >>>>
> >>>> This prevents a bricking issue for some Samsung devices but causes
> >>>> regression on many other machines.
> >>>>
> >>>> https://bugzilla.kernel.org/show_bug.cgi?id=55471
> >>>> https://bugzilla.redhat.com/show_bug.cgi?id=947142
> >>>> http://article.gmane.org/gmane.linux.kernel.efi/1078
> >>>> http://article.gmane.org/gmane.linux.kernel.stable/47150
> >>>>
> >>>> This patchset will fix it again:
> >>>>
> >>>> http://thread.gmane.org/gmane.linux.kernel.efi/1081
> >>>
> >>> Thanks for the pointer.  I note that in that thread you yourself imply
> >>> there are still issues after applying them?  Was that accurate, is there
> >>> yet further patches needed?
> >>
> >> I just find that issue. Didn't see that when writing the above.
> >>
> >> Earlier I was testing on a different machine and the result was
> >> good. I guess the patch still needs some more testing.
> > 
> > Thanks Lingzhu.
> > 
> > So, I guess you would recommend me to drop the whole series until we
> > have this patchset accepted, tested and back-ported for 3.5, correct?
> > 
> > (I tried to isolate the one you pointed out, but I'm afraid there are
> > too many dependencies between them to drop a single patch.)
> 
> Unfortunately yes.
> 
> The whole series are mostly solving pstore bugs and pstore trashing
> firmware. If you want to prevent more bricking from happening asap,
> a temporary choice is to allow disabling efi pstore altogether with these:
> 
> ed9dc8ce7a1c8115dba9483a9b51df8b63a2e0ef efivars: Allow disabling use as a pstore backend
> ec0971ba5372a4dfa753f232449d23a8fd98490e efivars: Add module parameter to disable use as a pstore backend
> ca0ba26fbbd2d81c43085df49ce0abfe34535a90 efivars: Fix check for CONFIG_EFI_VARS_PSTORE_DEFAULT_DISABLE

Thanks a lot for your help.  I'll just pick these and queue them for
3.5.y.

Cheers,
--
Luis

Patch

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index bfd8f43..9a1968e 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -943,6 +943,31 @@  static ssize_t efivar_delete(struct file *filp, struct kobject *kobj,
 }
 
 /*
+ * Returns the size of variable_name, in bytes, including the
+ * terminating NULL character, or variable_name_size if no NULL
+ * character is found among the first variable_name_size bytes.
+ */
+static unsigned long var_name_strnsize(efi_char16_t *variable_name,
+				       unsigned long variable_name_size)
+{
+	unsigned long len;
+	efi_char16_t c;
+
+	/*
+	 * The variable name is, by definition, a NULL-terminated
+	 * string, so make absolutely sure that variable_name_size is
+	 * the value we expect it to be. If not, return the real size.
+	 */
+	for (len = 2; len <= variable_name_size; len += sizeof(c)) {
+		c = variable_name[(len / sizeof(c)) - 1];
+		if (!c)
+			break;
+	}
+
+	return min(len, variable_name_size);
+}
+
+/*
  * Let's not leave out systab information that snuck into
  * the efivars driver
  */
@@ -1169,6 +1194,8 @@  int register_efivars(struct efivars *efivars,
 						&vendor_guid);
 		switch (status) {
 		case EFI_SUCCESS:
+			variable_name_size = var_name_strnsize(variable_name,
+							       variable_name_size);
 			efivar_create_sysfs_entry(efivars,
 						  variable_name_size,
 						  variable_name,