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  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

Powered by blists - more mailing lists