[PATCH] firmware: (memconsole) do not count numbers if read fails

[PATCH] firmware: (memconsole) do not count numbers if read fails

[Kangjie Lu]

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@umn.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;
 		pos -= seg[i].len;
 	}
 	return done;
-- 
2.17.2 (Apple Git-113)

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

[Greg Kroah-Hartman]

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@umn.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