| Message ID | 20181024083334.a6pxsifew3iztuun@kili.mountain (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3d39e1bb1c88f32820c5f9271f2c8c2fb9a52bac |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wireless: airo: potential buffer overflow in sprintf() | expand |
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.
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
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.
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.
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()
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,
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(-)