diff mbox

[OPW,kernel,v3] Staging: et131x: Fix Sparse Warning of Buffer Overflow

Message ID 1382440487-20874-1-git-send-email-rashika.kheria@gmail.com
State New, archived
Headers show

Commit Message

Rashika Oct. 22, 2013, 11:14 a.m. UTC
This patch fixes the following Sparse Warning in et131x.c-

drivers/staging/et131x/et131x.c:2957 nic_send_packet() error: buffer overflow 'frags' 17 <= 21
drivers/staging/et131x/et131x.c:2959 nic_send_packet() warn: buffer overflow 'frags' 17 <= 21
drivers/staging/et131x/et131x.c:2961 nic_send_packet() error: buffer overflow 'frags' 17 <= 21

Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
---

This revision fixes the following issues of the previous revision-
Incorrect Spacing

 drivers/staging/et131x/et131x.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Rusty Russell Oct. 24, 2013, 2:14 a.m. UTC | #1
Rashika Kheria <rashika.kheria@gmail.com> writes:
> This patch fixes the following Sparse Warning in et131x.c-
>
> drivers/staging/et131x/et131x.c:2957 nic_send_packet() error: buffer overflow 'frags' 17 <= 21
> drivers/staging/et131x/et131x.c:2959 nic_send_packet() warn: buffer overflow 'frags' 17 <= 21
> drivers/staging/et131x/et131x.c:2961 nic_send_packet() error: buffer overflow 'frags' 17 <= 21
>
> Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
> ---
>
> This revision fixes the following issues of the previous revision-
> Incorrect Spacing

Hi Rashika,

I can imagine this removes the sparse warning by confusing sparse, but I
don't see how this is correct.

sparse is complaining because of this:

	u32 nr_frags = skb_shinfo(skb)->nr_frags + 1;
	struct skb_frag_struct *frags = &skb_shinfo(skb)->frags[0];
        ...
	if (nr_frags > 23)
		return -EIO;
        ...
	for (i = 0; i < nr_frags; i++) {
		if (i == 0) {
                        ...
		} else {
			desc[frag].len_vlan = frags[i - 1].size;

So, sparse things nr_frags could be 22 (so i could be 22), and
skb_shinfo(skb)->frags[] is only 17 entries long.

Now, it turns out that sparse is wrong, but that's because the code is
wrong.

skb_shinfo(skb)->nr_frags can only be MAX_SKB_FRAGS (17).  But the code
is testing that nr_frags doesn't overflow 'struct tx_desc desc[24]',
which is why it tests for nr_frags > 23.

It would be sufficient to replace the  'if (nr_flags > 23)' clause with:

        /* nr_frags should be no more than 18. */
        BUILD_BUG_ON(MAX_SKB_FRAGS + 1 > 23);

That way, if anything changes in future, the compilation of this file
will fail.

Cheers,
Rusty.

>  drivers/staging/et131x/et131x.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
> index 7cd3d34..fdaeaf6 100644
> --- a/drivers/staging/et131x/et131x.c
> +++ b/drivers/staging/et131x/et131x.c
> @@ -2907,7 +2907,7 @@ static int nic_send_packet(struct et131x_adapter *adapter, struct tcb *tcb)
>  
>  	memset(desc, 0, sizeof(struct tx_desc) * (nr_frags + 1));
>  
> -	for (i = 0; i < nr_frags; i++) {
> +	for (i = 0; i < nr_frags - 1; i++) {
>  		/* If there is something in this element, lets get a
>  		 * descriptor from the ring and get the necessary data
>  		 */
> -- 
> 1.7.9.5
>
> -- 
> You received this message because you are subscribed to the Google Groups "opw-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to opw-kernel+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
diff mbox

Patch

diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
index 7cd3d34..fdaeaf6 100644
--- a/drivers/staging/et131x/et131x.c
+++ b/drivers/staging/et131x/et131x.c
@@ -2907,7 +2907,7 @@  static int nic_send_packet(struct et131x_adapter *adapter, struct tcb *tcb)
 
 	memset(desc, 0, sizeof(struct tx_desc) * (nr_frags + 1));
 
-	for (i = 0; i < nr_frags; i++) {
+	for (i = 0; i < nr_frags - 1; i++) {
 		/* If there is something in this element, lets get a
 		 * descriptor from the ring and get the necessary data
 		 */