[patch] HID: picoLCD: off by one in dump_buff_as_hex()

[patch] HID: picoLCD: off by one in dump_buff_as_hex()

[Dan Carpenter]

We're placing the NUL terminator one character beyond the end of the
buffer here.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
This change obviously don't make the code worse, but I'm not positive
it's the right fix.  I'm not sure the lines before are doing the right
thing either, if we had two chars of remaining space then wouldn't it be
better to put the new line and NUL in the unused space?

If you decide to do it differently, then please feel to sent a patch for
that and give me a Reported-by cookie.

diff --git a/drivers/hid/hid-picolcd_debugfs.c b/drivers/hid/hid-picolcd_debugfs.c
index eec85b5..ff271ff0 100644
--- a/drivers/hid/hid-picolcd_debugfs.c
+++ b/drivers/hid/hid-picolcd_debugfs.c
@@ -390,7 +390,7 @@ static void dump_buff_as_hex(char *dst, size_t dst_sz, const u8 *data,
 		dst[j--] = '\0';
 		dst[j] = '\n';
 	} else
-		dst[j] = '\0';
+		dst[dst_sz - 1] = '\0';
 }
 
 void picolcd_debug_out_report(struct picolcd_data *data,

Re: [patch] HID: picoLCD: off by one in dump_buff_as_hex()

[Bruno Prémont]

Hi Dan,

On Fri, 14 September 2012 Dan Carpenter <dan.carpenter@oracle.com> wrote:
> We're placing the NUL terminator one character beyond the end of the
> buffer here.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> This change obviously don't make the code worse, but I'm not positive
> it's the right fix.  I'm not sure the lines before are doing the right
> thing either, if we had two chars of remaining space then wouldn't it be
> better to put the new line and NUL in the unused space?

The output buffer full should not happen as the 256 bytes buffer can hold
more than the max expected report size of 64 bytes.

An option would be to convert to hex_dump_to_buffer(), but as this happens
partially under IRQ context - even though for debugging purposes - the
switch may be a bit too expensive, especially as one still has to append
linefeed after it.


I think I will extend your fix slightly in order to cover "too much input"
in a way visible to /sys/kernel/hid/*/events reader (for the case that
reports would grow in size).

Though no need to check for minimal output buffer size as callers are local
and feed in #defined sized buffer.


More on Wednesday evening, too late today and no time tomorrow.

Bruno

> If you decide to do it differently, then please feel to sent a patch for
> that and give me a Reported-by cookie.
> 
> diff --git a/drivers/hid/hid-picolcd_debugfs.c b/drivers/hid/hid-picolcd_debugfs.c
> index eec85b5..ff271ff0 100644
> --- a/drivers/hid/hid-picolcd_debugfs.c
> +++ b/drivers/hid/hid-picolcd_debugfs.c
> @@ -390,7 +390,7 @@ static void dump_buff_as_hex(char *dst, size_t dst_sz, const u8 *data,
>  		dst[j--] = '\0';
>  		dst[j] = '\n';
>  	} else
> -		dst[j] = '\0';
> +		dst[dst_sz - 1] = '\0';
>  }
>  
>  void picolcd_debug_out_report(struct picolcd_data *data,

Re: [patch] HID: picoLCD: off by one in dump_buff_as_hex()

[Bruno Prémont]

Dan,

What's your opinion on below alternative patch?
In addition to yours it makes would-overflow visible.

It does not check for output buffer having non-zero size but
as callers are local with #defined buffer size I don't think that would
be needed.





Author: Bruno Prémont <bonbons@linux-vserver.org>
Date:   Wed Sep 19 21:18:10 2012 +0200
Subject: HID: picoLCD: bounds check in dump_buff_as_hex()

Make sure we keep enough space for terminating NUL character after last
newline. If we have too much data, replace last byte with '.'s to
make overflow visible.

Using hex_dump_to_buffer() is not interesting as it adds more overhead
and does not append the trailing linefeed.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
---
 drivers/hid/hid-picolcd_debugfs.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-picolcd_debugfs.c b/drivers/hid/hid-picolcd_debugfs.c
index 868853a..c5c2fd9 100644
--- a/drivers/hid/hid-picolcd_debugfs.c
+++ b/drivers/hid/hid-picolcd_debugfs.c
@@ -381,16 +381,16 @@ static void dump_buff_as_hex(char *dst, size_t dst_sz, const u8 *data,
 		const size_t data_len)
 {
 	int i, j;
-	for (i = j = 0; i < data_len && j + 3 < dst_sz; i++) {
+	for (i = j = 0; i < data_len && j + 4 < dst_sz; i++) {
 		dst[j++] = hex_asc[(data[i] >> 4) & 0x0f];
 		dst[j++] = hex_asc[data[i] & 0x0f];
 		dst[j++] = ' ';
 	}
-	if (j < dst_sz) {
-		dst[j--] = '\0';
-		dst[j] = '\n';
-	} else
-		dst[j] = '\0';
+	dst[j]   = '\0';
+	if (j > 0)
+		dst[j-1] = '\n';
+	if (i < data_len && j > 2)
+		dst[j-2] = dst[j-3] = '.';
 }
 
 void picolcd_debug_out_report(struct picolcd_data *data,
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] HID: picoLCD: off by one in dump_buff_as_hex()

[Dan Carpenter]

On Wed, Sep 19, 2012 at 09:35:35PM +0200, Bruno Prémont wrote:
> Dan,
> 
> What's your opinion on below alternative patch?
> In addition to yours it makes would-overflow visible.
> 
> It does not check for output buffer having non-zero size but
> as callers are local with #defined buffer size I don't think that would
> be needed.
> 

Sorry for the delay.  Looks good to me.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] HID: picoLCD: off by one in dump_buff_as_hex()

[Jiri Kosina]

On Sat, 22 Sep 2012, Dan Carpenter wrote:

> > What's your opinion on below alternative patch?
> > In addition to yours it makes would-overflow visible.
> > 
> > It does not check for output buffer having non-zero size but
> > as callers are local with #defined buffer size I don't think that would
> > be needed.
> > 
> 
> Sorry for the delay.  Looks good to me.

I am picking Bruno's patch. Thanks,

-- 
Jiri Kosina
SUSE Labs