linux-kernel - Re: [PATCH] firmware: (memconsole) do not count numbers if read fails

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 22 Jan 2019 10:27:06 +0100
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Kangjie Lu <kjlu@....edu>
Cc:     pakki001@....edu, Samuel Holland <samuel@...lland.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] firmware: (memconsole) do not count numbers if read fails

On Tue, Dec 25, 2018 at 11:29:11PM -0600, Kangjie Lu wrote:
> When memory_read_from_buffer() fails, the return value is a negative
> error code, thus we shouldn't count it as the number of read bytes.
> 
> The fix checks the return value of memory_read_from_buffer, and count
> the number only when it succeeds.
> 
> Signed-off-by: Kangjie Lu <kjlu@....edu>
> ---
>  drivers/firmware/google/memconsole-coreboot.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/google/memconsole-coreboot.c b/drivers/firmware/google/memconsole-coreboot.c
> index b29e10757bfb..4e8a0ad110c1 100644
> --- a/drivers/firmware/google/memconsole-coreboot.c
> +++ b/drivers/firmware/google/memconsole-coreboot.c
> @@ -55,6 +55,7 @@ static ssize_t memconsole_coreboot_read(char *buf, loff_t pos, size_t count)
>  	} seg[2] = { {0}, {0} };
>  	size_t done = 0;
>  	int i;
> +	int ret;
>  
>  	if (flags & OVERFLOW) {
>  		if (cursor > size)	/* Shouldn't really happen, but... */
> @@ -66,8 +67,10 @@ static ssize_t memconsole_coreboot_read(char *buf, loff_t pos, size_t count)
>  	}
>  
>  	for (i = 0; i < ARRAY_SIZE(seg) && count > done; i++) {
> -		done += memory_read_from_buffer(buf + done, count - done, &pos,
> +		ret = memory_read_from_buffer(buf + done, count - done, &pos,
>  			cbmem_console->body + seg[i].phys, seg[i].len);
> +		if (ret >= 0)
> +			done += ret;

But if ret < 0, then it's an error, and something should happen, right?

And really, the only way this can fail is if pos is less than 0, which.
And if that happens, you just now stuck us in an endless loop, which is
worse than just ignoring the error value returned :(

thanks,

greg k-h