| Message ID | 20220708071903.25752-1-chunfeng.yun@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] usb: mtu3: fix coverity of string buffer | expand |
On Fri, 2022-07-08 at 09:26 +0200, Greg Kroah-Hartman wrote: > On Fri, Jul 08, 2022 at 03:18:59PM +0800, Chunfeng Yun wrote: > > Use snprintf instead of sprintf which could cause buffer overflow. > > How can it cause an overflow? Maybe I didn't describe it clearly, this patch is used to fix coverity check warning of string buffer, in fact no overflow happens. > > > > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> > > --- > > drivers/usb/mtu3/mtu3.h | 4 +++- > > drivers/usb/mtu3/mtu3_debugfs.c | 2 +- > > drivers/usb/mtu3/mtu3_gadget.c | 4 ++-- > > 3 files changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/usb/mtu3/mtu3.h b/drivers/usb/mtu3/mtu3.h > > index 8408e1b1a24a..9893dd1bafbb 100644 > > --- a/drivers/usb/mtu3/mtu3.h > > +++ b/drivers/usb/mtu3/mtu3.h > > @@ -92,6 +92,8 @@ struct mtu3_request; > > > > #define BULK_CLKS_CNT 4 > > > > +#define MTU3_EP_NAME_LEN 12 > > + > > /* device operated link and speed got from DEVICE_CONF register */ > > enum mtu3_speed { > > MTU3_SPEED_INACTIVE = 0, > > @@ -272,7 +274,7 @@ struct ssusb_mtk { > > */ > > struct mtu3_ep { > > struct usb_ep ep; > > - char name[12]; > > + char name[MTU3_EP_NAME_LEN]; > > struct mtu3 *mtu; > > u8 epnum; > > u8 type; > > diff --git a/drivers/usb/mtu3/mtu3_debugfs.c > > b/drivers/usb/mtu3/mtu3_debugfs.c > > index d27de647c86a..a6f72494b819 100644 > > --- a/drivers/usb/mtu3/mtu3_debugfs.c > > +++ b/drivers/usb/mtu3/mtu3_debugfs.c > > @@ -132,7 +132,7 @@ static void mtu3_debugfs_regset(struct mtu3 > > *mtu, void __iomem *base, > > if (!mregs) > > return; > > > > - sprintf(mregs->name, "%s", name); > > + snprintf(mregs->name, MTU3_DEBUGFS_NAME_LEN, "%s", name); > > Where does name come from? It looks like you control this string, so > there is no overflow anywhere. Yes, don't encounter overflow at all, but there is warning when scanned by coverity in our internal project branches. > > > regset = &mregs->regset; > > regset->regs = regs; > > regset->nregs = nregs; > > diff --git a/drivers/usb/mtu3/mtu3_gadget.c > > b/drivers/usb/mtu3/mtu3_gadget.c > > index 30999b4debb8..a751e0533c2d 100644 > > --- a/drivers/usb/mtu3/mtu3_gadget.c > > +++ b/drivers/usb/mtu3/mtu3_gadget.c > > @@ -635,8 +635,8 @@ static void init_hw_ep(struct mtu3 *mtu, struct > > mtu3_ep *mep, > > > > INIT_LIST_HEAD(&mep->req_list); > > > > - sprintf(mep->name, "ep%d%s", epnum, > > - !epnum ? "" : (is_in ? "in" : "out")); > > + snprintf(mep->name, MTU3_EP_NAME_LEN, "ep%d%s", epnum, > > + !epnum ? "" : (is_in ? "in" : "out")); > > Same here, you already control this string size, so where is the > issue? Just fix coverity warning. Thanks a lot > > thanks, > > greg k-h
On Mon, Jul 11, 2022 at 02:39:10PM +0800, Chunfeng Yun wrote: > On Fri, 2022-07-08 at 09:26 +0200, Greg Kroah-Hartman wrote: > > On Fri, Jul 08, 2022 at 03:18:59PM +0800, Chunfeng Yun wrote: > > > Use snprintf instead of sprintf which could cause buffer overflow. > > > > How can it cause an overflow? > Maybe I didn't describe it clearly, this patch is used to fix coverity > check warning of string buffer, in fact no overflow happens. Then perhaps the coverity warning is useless and yet-another-false-positive that that tool is known to spit out? Don't make code changes just because broken tools say to do so. thanks, greg k-h
On Mon, 2022-07-11 at 08:45 +0200, Greg Kroah-Hartman wrote: > On Mon, Jul 11, 2022 at 02:39:10PM +0800, Chunfeng Yun wrote: > > On Fri, 2022-07-08 at 09:26 +0200, Greg Kroah-Hartman wrote: > > > On Fri, Jul 08, 2022 at 03:18:59PM +0800, Chunfeng Yun wrote: > > > > Use snprintf instead of sprintf which could cause buffer > > > > overflow. > > > > > > How can it cause an overflow? > > > > Maybe I didn't describe it clearly, this patch is used to fix > > coverity > > check warning of string buffer, in fact no overflow happens. > > Then perhaps the coverity warning is useless and > yet-another-false-positive that that tool is known to spit out? > > Don't make code changes just because broken tools say to do so. Got it, thanks a lot > > thanks, > > greg k-h
diff --git a/drivers/usb/mtu3/mtu3.h b/drivers/usb/mtu3/mtu3.h index 8408e1b1a24a..9893dd1bafbb 100644 --- a/drivers/usb/mtu3/mtu3.h +++ b/drivers/usb/mtu3/mtu3.h @@ -92,6 +92,8 @@ struct mtu3_request; #define BULK_CLKS_CNT 4 +#define MTU3_EP_NAME_LEN 12 + /* device operated link and speed got from DEVICE_CONF register */ enum mtu3_speed { MTU3_SPEED_INACTIVE = 0, @@ -272,7 +274,7 @@ struct ssusb_mtk { */ struct mtu3_ep { struct usb_ep ep; - char name[12]; + char name[MTU3_EP_NAME_LEN]; struct mtu3 *mtu; u8 epnum; u8 type; diff --git a/drivers/usb/mtu3/mtu3_debugfs.c b/drivers/usb/mtu3/mtu3_debugfs.c index d27de647c86a..a6f72494b819 100644 --- a/drivers/usb/mtu3/mtu3_debugfs.c +++ b/drivers/usb/mtu3/mtu3_debugfs.c @@ -132,7 +132,7 @@ static void mtu3_debugfs_regset(struct mtu3 *mtu, void __iomem *base, if (!mregs) return; - sprintf(mregs->name, "%s", name); + snprintf(mregs->name, MTU3_DEBUGFS_NAME_LEN, "%s", name); regset = &mregs->regset; regset->regs = regs; regset->nregs = nregs; diff --git a/drivers/usb/mtu3/mtu3_gadget.c b/drivers/usb/mtu3/mtu3_gadget.c index 30999b4debb8..a751e0533c2d 100644 --- a/drivers/usb/mtu3/mtu3_gadget.c +++ b/drivers/usb/mtu3/mtu3_gadget.c @@ -635,8 +635,8 @@ static void init_hw_ep(struct mtu3 *mtu, struct mtu3_ep *mep, INIT_LIST_HEAD(&mep->req_list); - sprintf(mep->name, "ep%d%s", epnum, - !epnum ? "" : (is_in ? "in" : "out")); + snprintf(mep->name, MTU3_EP_NAME_LEN, "ep%d%s", epnum, + !epnum ? "" : (is_in ? "in" : "out")); mep->ep.name = mep->name; INIT_LIST_HEAD(&mep->ep.ep_list);
Use snprintf instead of sprintf which could cause buffer overflow. Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> --- drivers/usb/mtu3/mtu3.h | 4 +++- drivers/usb/mtu3/mtu3_debugfs.c | 2 +- drivers/usb/mtu3/mtu3_gadget.c | 4 ++-- 3 files changed, 6 insertions(+), 4 deletions(-)