[PATCH] wireless: airo: potential buffer overflow in sprintf()

[PATCH] wireless: airo: potential buffer overflow in sprintf()

[Dan Carpenter]

It looks like we wanted to print a maximum of BSSList_rid.ssidLen bytes
of the ssid, but we accidentally use "%*s" (width) instead of "%.*s"
(precision) so if the ssid doesn't have a NUL terminator this could lead
to an overflow.

Fixes: e174961ca1a0 ("net: convert print_mac to %pM")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Static analsysis.  Not tested.

 drivers/net/wireless/cisco/airo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/cisco/airo.c b/drivers/net/wireless/cisco/airo.c
index 04dd7a936593..5512c7f73fce 100644
--- a/drivers/net/wireless/cisco/airo.c
+++ b/drivers/net/wireless/cisco/airo.c
@@ -5462,7 +5462,7 @@ static int proc_BSSList_open( struct inode *inode, struct file *file ) {
            we have to add a spin lock... */
 	rc = readBSSListRid(ai, doLoseSync, &BSSList_rid);
 	while(rc == 0 && BSSList_rid.index != cpu_to_le16(0xffff)) {
-		ptr += sprintf(ptr, "%pM %*s rssi = %d",
+		ptr += sprintf(ptr, "%pM %.*s rssi = %d",
 			       BSSList_rid.bssid,
 				(int)BSSList_rid.ssidLen,
 				BSSList_rid.ssid,
-- 
2.11.0

[PATCH] wireless: airo: potential buffer overflow in sprintf()

[Dan Carpenter]

It looks like we wanted to print a maximum of BSSList_rid.ssidLen bytes
of the ssid, but we accidentally use "%*s" (width) instead of "%.*s"
(precision) so if the ssid doesn't have a NUL terminator this could lead
to an overflow.

Fixes: e174961ca1a0 ("net: convert print_mac to %pM")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Static analsysis.  Not tested.

 drivers/net/wireless/cisco/airo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/cisco/airo.c b/drivers/net/wireless/cisco/airo.c
index 04dd7a936593..5512c7f73fce 100644
--- a/drivers/net/wireless/cisco/airo.c
+++ b/drivers/net/wireless/cisco/airo.c
@@ -5462,7 +5462,7 @@ static int proc_BSSList_open( struct inode *inode, struct file *file ) {
            we have to add a spin lock... */
 	rc = readBSSListRid(ai, doLoseSync, &BSSList_rid);
 	while(rc = 0 && BSSList_rid.index != cpu_to_le16(0xffff)) {
-		ptr += sprintf(ptr, "%pM %*s rssi = %d",
+		ptr += sprintf(ptr, "%pM %.*s rssi = %d",
 			       BSSList_rid.bssid,
 				(int)BSSList_rid.ssidLen,
 				BSSList_rid.ssid,
-- 
2.11.0

Re: [PATCH] wireless: airo: potential buffer overflow in sprintf()

[Kalle Valo]

Dan Carpenter <dan.carpenter@oracle.com> writes:

> It looks like we wanted to print a maximum of BSSList_rid.ssidLen bytes
> of the ssid, but we accidentally use "%*s" (width) instead of "%.*s"
> (precision) so if the ssid doesn't have a NUL terminator this could lead
> to an overflow.
>
> Fixes: e174961ca1a0 ("net: convert print_mac to %pM")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> Static analsysis.  Not tested.

IMHO this part (after "---" line) is important information and should be
part of commit log. I can fix that.

I'll also remove "wireless:" from the title.

-- 
Kalle Valo

Re: [PATCH] wireless: airo: potential buffer overflow in sprintf()

[Kalle Valo]

Dan Carpenter <dan.carpenter@oracle.com> writes:

> It looks like we wanted to print a maximum of BSSList_rid.ssidLen bytes
> of the ssid, but we accidentally use "%*s" (width) instead of "%.*s"
> (precision) so if the ssid doesn't have a NUL terminator this could lead
> to an overflow.
>
> Fixes: e174961ca1a0 ("net: convert print_mac to %pM")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> Static analsysis.  Not tested.

IMHO this part (after "---" line) is important information and should be
part of commit log. I can fix that.

I'll also remove "wireless:" from the title.

-- 
Kalle Valo

Re: [PATCH] wireless: airo: potential buffer overflow in sprintf()

[Dan Carpenter]

On Wed, Oct 24, 2018 at 11:56:53AM +0300, Kalle Valo wrote:
> Dan Carpenter <dan.carpenter@oracle.com> writes:
> 
> > It looks like we wanted to print a maximum of BSSList_rid.ssidLen bytes
> > of the ssid, but we accidentally use "%*s" (width) instead of "%.*s"
> > (precision) so if the ssid doesn't have a NUL terminator this could lead
> > to an overflow.
> >
> > Fixes: e174961ca1a0 ("net: convert print_mac to %pM")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > Static analsysis.  Not tested.
> 
> IMHO this part (after "---" line) is important information and should be
> part of commit log. I can fix that.
> 

In my experience most maintainers disagree (with varying degrees of
intensity).

regards,
dan carpenter

Re: [PATCH] wireless: airo: potential buffer overflow in sprintf()

[Dan Carpenter]

On Wed, Oct 24, 2018 at 11:56:53AM +0300, Kalle Valo wrote:
> Dan Carpenter <dan.carpenter@oracle.com> writes:
> 
> > It looks like we wanted to print a maximum of BSSList_rid.ssidLen bytes
> > of the ssid, but we accidentally use "%*s" (width) instead of "%.*s"
> > (precision) so if the ssid doesn't have a NUL terminator this could lead
> > to an overflow.
> >
> > Fixes: e174961ca1a0 ("net: convert print_mac to %pM")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > Static analsysis.  Not tested.
> 
> IMHO this part (after "---" line) is important information and should be
> part of commit log. I can fix that.
> 

In my experience most maintainers disagree (with varying degrees of
intensity).

regards,
dan carpenter

Re: [PATCH] wireless: airo: potential buffer overflow in sprintf()

[Kalle Valo]

Dan Carpenter <dan.carpenter@oracle.com> writes:

> On Wed, Oct 24, 2018 at 11:56:53AM +0300, Kalle Valo wrote:
>> Dan Carpenter <dan.carpenter@oracle.com> writes:
>> 
>> > It looks like we wanted to print a maximum of BSSList_rid.ssidLen bytes
>> > of the ssid, but we accidentally use "%*s" (width) instead of "%.*s"
>> > (precision) so if the ssid doesn't have a NUL terminator this could lead
>> > to an overflow.
>> >
>> > Fixes: e174961ca1a0 ("net: convert print_mac to %pM")
>> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>> > ---
>> > Static analsysis.  Not tested.
>> 
>> IMHO this part (after "---" line) is important information and should be
>> part of commit log. I can fix that.
>> 
>
> In my experience most maintainers disagree (with varying degrees of
> intensity).

Heh, why would adding four words explaining the background of the patch
to a commit log would be a bad thing? :) Well, I guess I just view
things differently.

-- 
Kalle Valo

Re: [PATCH] wireless: airo: potential buffer overflow in sprintf()

[Kalle Valo]

Dan Carpenter <dan.carpenter@oracle.com> writes:

> On Wed, Oct 24, 2018 at 11:56:53AM +0300, Kalle Valo wrote:
>> Dan Carpenter <dan.carpenter@oracle.com> writes:
>> 
>> > It looks like we wanted to print a maximum of BSSList_rid.ssidLen bytes
>> > of the ssid, but we accidentally use "%*s" (width) instead of "%.*s"
>> > (precision) so if the ssid doesn't have a NUL terminator this could lead
>> > to an overflow.
>> >
>> > Fixes: e174961ca1a0 ("net: convert print_mac to %pM")
>> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>> > ---
>> > Static analsysis.  Not tested.
>> 
>> IMHO this part (after "---" line) is important information and should be
>> part of commit log. I can fix that.
>> 
>
> In my experience most maintainers disagree (with varying degrees of
> intensity).

Heh, why would adding four words explaining the background of the patch
to a commit log would be a bad thing? :) Well, I guess I just view
things differently.

-- 
Kalle Valo

Re: [PATCH] wireless: airo: potential buffer overflow in sprintf()

[Amir Goldstein]

On Wed, Oct 24, 2018 at 12:23 PM Kalle Valo <kvalo@codeaurora.org> wrote:
>
> Dan Carpenter <dan.carpenter@oracle.com> writes:
>
> > On Wed, Oct 24, 2018 at 11:56:53AM +0300, Kalle Valo wrote:
> >> Dan Carpenter <dan.carpenter@oracle.com> writes:
> >>
> >> > It looks like we wanted to print a maximum of BSSList_rid.ssidLen bytes
> >> > of the ssid, but we accidentally use "%*s" (width) instead of "%.*s"
> >> > (precision) so if the ssid doesn't have a NUL terminator this could lead
> >> > to an overflow.
> >> >
> >> > Fixes: e174961ca1a0 ("net: convert print_mac to %pM")
> >> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >> > ---
> >> > Static analsysis.  Not tested.
> >>
> >> IMHO this part (after "---" line) is important information and should be
> >> part of commit log. I can fix that.
> >>
> >
> > In my experience most maintainers disagree (with varying degrees of
> > intensity).
>
> Heh, why would adding four words explaining the background of the patch
> to a commit log would be a bad thing? :) Well, I guess I just view
> things differently.
>

By the time a maintainer applies a patch and requests to merge it upstream
the patch should be tested. Right?
So how would a comment "Not tested" make any sense in an upstream
merged patch?

Thanks,
Amir.

Re: [PATCH] wireless: airo: potential buffer overflow in sprintf()

[Amir Goldstein]

On Wed, Oct 24, 2018 at 12:23 PM Kalle Valo <kvalo@codeaurora.org> wrote:
>
> Dan Carpenter <dan.carpenter@oracle.com> writes:
>
> > On Wed, Oct 24, 2018 at 11:56:53AM +0300, Kalle Valo wrote:
> >> Dan Carpenter <dan.carpenter@oracle.com> writes:
> >>
> >> > It looks like we wanted to print a maximum of BSSList_rid.ssidLen bytes
> >> > of the ssid, but we accidentally use "%*s" (width) instead of "%.*s"
> >> > (precision) so if the ssid doesn't have a NUL terminator this could lead
> >> > to an overflow.
> >> >
> >> > Fixes: e174961ca1a0 ("net: convert print_mac to %pM")
> >> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >> > ---
> >> > Static analsysis.  Not tested.
> >>
> >> IMHO this part (after "---" line) is important information and should be
> >> part of commit log. I can fix that.
> >>
> >
> > In my experience most maintainers disagree (with varying degrees of
> > intensity).
>
> Heh, why would adding four words explaining the background of the patch
> to a commit log would be a bad thing? :) Well, I guess I just view
> things differently.
>

By the time a maintainer applies a patch and requests to merge it upstream
the patch should be tested. Right?
So how would a comment "Not tested" make any sense in an upstream
merged patch?

Thanks,
Amir.

Re: [PATCH] wireless: airo: potential buffer overflow in sprintf()

[Kalle Valo]

Dan Carpenter <dan.carpenter@oracle.com> wrote:

> It looks like we wanted to print a maximum of BSSList_rid.ssidLen bytes
> of the ssid, but we accidentally use "%*s" (width) instead of "%.*s"
> (precision) so if the ssid doesn't have a NUL terminator this could lead
> to an overflow.
> 
> Static analysis.  Not tested.
> 
> Fixes: e174961ca1a0 ("net: convert print_mac to %pM")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Patch applied to wireless-drivers-next.git, thanks.

3d39e1bb1c88 wireless: airo: potential buffer overflow in sprintf()

-- 
https://patchwork.kernel.org/patch/10654389/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] wireless: airo: potential buffer overflow in sprintf()

[Kalle Valo]

Dan Carpenter <dan.carpenter@oracle.com> wrote:

> It looks like we wanted to print a maximum of BSSList_rid.ssidLen bytes
> of the ssid, but we accidentally use "%*s" (width) instead of "%.*s"
> (precision) so if the ssid doesn't have a NUL terminator this could lead
> to an overflow.
> 
> Static analysis.  Not tested.
> 
> Fixes: e174961ca1a0 ("net: convert print_mac to %pM")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Patch applied to wireless-drivers-next.git, thanks.

3d39e1bb1c88 wireless: airo: potential buffer overflow in sprintf()

-- 
https://patchwork.kernel.org/patch/10654389/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches