Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 4 Mar 2020 14:51:22 +0800
From:   joeyli <jlee@suse.com>
To:     Vladis Dronov <vdronov@redhat.com>
CC:     Ard Biesheuvel <ardb@kernel.org>,
        linux-efi <linux-efi@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] efi: fix a race and a buffer overflow while reading
 efivars via sysfs
Message-ID: <20200304065122.GK16878@linux-l9pv.suse>
References: <20200303085528.27658-1-vdronov@redhat.com>
 <CAKv+Gu_3ZRRcoAcLTVVQe26q5x9KALmztaNQF=e=KqWaAwxtpA@mail.gmail.com>
 <1980156503.12639063.1583230452485.JavaMail.zimbra@redhat.com>
 <358842423.12639861.1583231098544.JavaMail.zimbra@redhat.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <358842423.12639861.1583231098544.JavaMail.zimbra@redhat.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
Received: from linux-l9pv.suse (60.251.47.115) by HK2PR03CA0058.apcprd03.prod.outlook.com (2603:1096:202:17::28) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2793.5 via Frontend Transport; Wed, 4 Mar 2020 06:51:29 +0000
X-Originating-IP: [60.251.47.115]
X-MS-PublicTrafficType: Email
X-MS-Office365-Filtering-Correlation-Id: 4f71b63c-e37c-4696-b892-08d7c0087846
X-MS-TrafficTypeDiagnostic: CH2PR18MB3351:
X-Microsoft-Antispam-PRVS: <CH2PR18MB3351025DCB5604F1138383DCA3E50@CH2PR18MB3351.namprd18.prod.outlook.com>
X-MS-Oob-TLC-OOBClassifiers: OLM:9508;
X-Forefront-PRVS: 0332AACBC3
X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10019020)(4636009)(39860400002)(366004)(346002)(136003)(376002)(396003)(199004)(189003)(52116002)(9686003)(55016002)(33656002)(8886007)(36756003)(7696005)(1076003)(5660300002)(6666004)(956004)(66946007)(6506007)(6916009)(16526019)(66556008)(66476007)(316002)(478600001)(54906003)(186003)(4326008)(55236004)(81166006)(81156014)(26005)(86362001)(2906002)(8676002)(8936002)(43062003);DIR:OUT;SFP:1102;SCL:1;SRVR:CH2PR18MB3351;H:CH2PR18MB3240.namprd18.prod.outlook.com;FPR:;SPF:None;LANG:en;PTR:InfoNoRecords;A:1;MX:1;
Received-SPF: None (protection.outlook.com: suse.com does not designate
 permitted sender hosts)
X-MS-Exchange-SenderADCheck: 1
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info: Q5tYLrL/mVfnamlq74XVIFpZ0um/50b9L4o5JNV1vkW/1O1nB0bh0ffkEcRIp7HfXNCFsSU3WRORyraUDXk+PCv444HqZ13NEN3k9WGauXd1n63iTyW1iC7cCHWLHl6zbEXiqYQLKttajFovO1PvmmCug61WXb13hPhKMMfpu+QBycrdFiTXp/FayaZn+Kv2DPP+oMVqstC8+vnBb/CSbwKEiTpMdmosOUdnQ4CtXbxlpCnd/zlTUSbXyYRhMFNzbxlp76Xx1vXfMKe7aJO4AfUdYKlE4mnmCGG9Ch0pKQPfx88YBEm65O6cQs06DtLZ0hlLOrRvDF8s//t0r3cI7lNmbArBECcGv8i6+E51sGspEJrIiQbbUUeP8yAntOLKHkFRAcs1LmAdONwAT5UJbBaVAWLLU4Sqc3qvtWS5ig54dv7PnUpqnc3o3rzKAtKzDdCi+gcFXKMKrzBJ1s4RqQvQ6J2qd0uv4bflx4DhQ3MMWnq5w6dlRB89JOZkF8c5
X-MS-Exchange-AntiSpam-MessageData: oLy1pVK6zaNsWQ54lD1MCOQnvH5LwhzzVnLLux8FDhCKsq1qVMGzJRuqlAM8bdqA3O3WXPlqK1jAPHQdCyS5CDmpyqn4dVNi72EQNnUzNdVkqaA6bw5BzgnXtLdJO5OpX7f3qMLKcG+6hYEQ2/G8sg==
X-MS-Exchange-CrossTenant-Network-Message-Id: 4f71b63c-e37c-4696-b892-08d7c0087846
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Mar 2020 06:51:31.0004
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 856b813c-16e5-49a5-85ec-6f081e13b527
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: L3HNTQGO/bAB5IjnhnNsBUVU5YNFgn2xthlibAk16rOsnYBu1eFJ9I9Z9vMr/oUp
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH2PR18MB3351
X-OriginatorOrg: suse.com
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi all,

On Tue, Mar 03, 2020 at 05:24:58AM -0500, Vladis Dronov wrote:
> Hello, Ard, all,
> 
> > > Wouldn't it be easier to pass a var_data_size stack variable into
> > > efivar_entry_get(), and only update the value in 'var' if it is <=
> > > 1024?
> > > 
> > 
> > I was thinking about this approach, but this way we still do not protect
> > var from a concurrent access. For example, efivar_data_read() can race
> > with itself:
> 
> Oh, indeed, this race is not possible the way you sugget with a var_data_size
> stack variable. Unfortunately, AFAIU, the read/write race stays:
>  
> > ... efivar read functions still can race with the write function
> > efivar_store_raw(). Surely, the race window is much smaller but it is there.
> > I strongly believe we need to protect all data accesses here with a lock.
>

Looks that kernel uses EFI protocol to query variable everytime, then
why should kernel keeps a copy of variable data size, data and attributes in
memory? It makes sense to keep VariableName and VendorGuid, but why data?

The efi_variable can be used to interactive with userland. But we do not
need to keep a data copy in efi_variable with efivar_entry. e.g. The
efivarfs_file_read() allocates a buffer for reading variable instead
of using efi_variable->Data. 

Regards
Joey Lee