From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1ZtjTB-0003ou-4Z for mharc-grub-devel@gnu.org; Tue, 03 Nov 2015 16:50:21 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42160) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZtjLd-0005E1-1i for grub-devel@gnu.org; Tue, 03 Nov 2015 16:42:33 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZtjLX-0002k5-S8 for grub-devel@gnu.org; Tue, 03 Nov 2015 16:42:32 -0500 Received: from mout.gmx.net ([212.227.15.19]:52125) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZtjLX-0002jj-Hc for grub-devel@gnu.org; Tue, 03 Nov 2015 16:42:27 -0500 Received: from [192.168.4.129] ([130.180.109.134]) by mail.gmx.com (mrgmx003) with ESMTPSA (Nemesis) id 0M7HGA-1ai0HA47WQ-00x5Az; Tue, 03 Nov 2015 22:42:23 +0100 Subject: Re: [coreboot] Dell Dimension 8300 reboots when grub2 cbfs module is loaded To: Andrei Borzenkov , The development of GNU GRUB , coreboot@coreboot.org References: <56362757.3070801@gmail.com> From: Nico Huber Message-ID: <563929F3.7050809@gmx.de> Date: Tue, 3 Nov 2015 22:41:07 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: <56362757.3070801@gmail.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-Provags-ID: V03:K0:Gg1fbRdDCWVPbcWAPDLJ/ppVidKSAWhPC0Jmecrw82be6aDVaPa L93bEBNlPaEhDZP7OwwSLKJN07PLO0uaN9NzZuoAjMnYXDpN3+9EzCEjGAsmRw5nRoC8LR7 Q7PFO9dkY48G5C9P0FFPJL6iSiBh6Fa/xU86oVKtijoZJRxujVE7Mjqe39r7Kkjh1A+b/ul kd0zWuoAik7divAILu+Ug== X-UI-Out-Filterresults: notjunk:1;V01:K0:vMsj7gEZJ2g=:xwRhXVlH2+2e1d2voi8MCc BzHpHHTw2/psrSkzQnHDhRKTkC0Fupev6j6wQmqv8UdxtjYLA24B+cOWwjxNaHlaaoBU0pu/Q REeGqcLIinLSric33P+l2hW6A0iWXGJqVz5tnCErQq08pH9Fxk0kj+je6/22DcqdwRCRv6Hkn wXrMWPthPtI/b7hG+soJtFxZBs7O+TVLdRjcRFOl0vbnDZwYOPSzJJBANd4UHkQb53nAp9i+E IVzxBUfbe/70rCUqWXJeIVpjpvvOETBFhwAOg9zKjr549W1u7VOXFZoc4EoXKAT7K7RVM8ElM JCUzZ5+eKujBLyPO8uqlgYbk+2sMQObs3czaveToPmYJ2zYA5N2Tq7Muo1S4CQ0B+yRo8Z2f4 4mcbmMzBiiWVLQJOKSGIF6WjTiN+BVeO3Ula5HGDe1Toaz27q5wSanwDOJO8NBKHt14kaYMBw gNoX0X9hypDo5pRkjJhCyVIMWfSfExHov5VIs3kLAp5qVRoRLymRK/SvqDqakc2t4/CtqSauD h7mpkqN0C5vPffDpQ4gYyOD22A9NDOyFEAe0teTmockEadLEQFblo5brFG0kfvC5zbMi4aUXe Bpb431GxwPlEvorCn5KmpAGuQk4Y5ftFOL0LJcku8iCMxtEHW3O6FaChWwU7la+IP+yMP3x// 2DaaWvbEsOOo4GlfudR96Xf0eyUkEG+mEconNraR03bF6sQbtYMYklfTMVKnGXUMNENbRkZN0 nniYlIpHor80lTp0E6mT/c8ephgMrWurIbrwCOf1jf6qfRxoPlVD1k3nVAgUPChhpSA2Lxg8v 0slz8Nm X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 212.227.15.19 X-Mailman-Approved-At: Tue, 03 Nov 2015 16:50:19 -0500 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 03 Nov 2015 21:42:34 -0000 Hi Andrei, your patch looks good generally, but the check is off by one. It's obvious, we want to have sane checking there. Reading from a random address can cause trouble and 0xffffffff is not the only offending address. On x86, the cbfs is mapped right below the 4GiB line. Current machines don't have more than 16MiB space for cbfs, FWIW. So maybe it's best to check if the ptr points somewhere between 0xff000000 and (0x100000000 - sizeof(*head)). Nico On 01.11.2015 15:53, Andrei Borzenkov wrote: > I was debugging problem reported by user on Dell Dimension 8300 - it > rebooted when doing "ls -l". It turned out, the problem was triggered by > loading cbfs which probed for header. System has 2GB memory, and attempt > to read from address 0xffffffff caused instant reboot. 0xffffffff was > returned by read from non-existing address 0xfffffffc. > > The proof of concept patch below avoids it, but I wonder what the proper > fix is. > > diff --git a/grub-core/fs/cbfs.c b/grub-core/fs/cbfs.c > index a34eb88..a5a2fde 100644 > --- a/grub-core/fs/cbfs.c > +++ b/grub-core/fs/cbfs.c > @@ -344,8 +344,9 @@ init_cbfsdisk (void) > > ptr = *(grub_uint32_t *) 0xfffffffc; > head = (struct cbfs_header *) (grub_addr_t) ptr; > + grub_dprintf ("cbfs", "head=%p\n", head); > > - if (!validate_head (head)) > + if (0xffffffff - ptr < sizeof (*head) || !validate_head (head)) > return; > > cbfsdisk_size = ALIGN_UP (grub_be_to_cpu32 (head->romsize), > >