Re: [PATCH RESEND net] nfc: use GFP_USER for user-controlled kmalloc
On Wed, Jan 27, 2016 at 1:05 PM, Eric Dumazetwrote: > On Wed, 2016-01-27 at 11:50 -0800, Cong Wang wrote: > >> Hmm? I think nfc_llcp_send_ui_frame() needs to do some fragmention >> with this temporary memory, or you are saying msg_iter has some >> API available to seek the pointer? Even if so, it doesn't look like >> suitable for -stable. >> > > memcpy_from_msg(msg_data, msg, len) will overwrite the msg_data with len > bytes, or return an error. > > So prior msg_data content does not matter. > > kzalloc() before a memset() or memcpy() sounds defensive programming, > kmalloc() is a bit faster. Oh, you mean s/kzalloc/kmalloc/, I thought you mean s/kzalloc//. ;)
Re: [PATCH RESEND net] nfc: use GFP_USER for user-controlled kmalloc
On Tue, Jan 26, 2016 at 6:23 PM, Eric Dumazetwrote: > On Tue, 2016-01-26 at 15:12 -0800, Cong Wang wrote: >> On Tue, Jan 26, 2016 at 2:55 PM, Julian Calaby >> wrote: >> > Hi Cong, >> > >> > On Wed, Jan 27, 2016 at 4:53 AM, Cong Wang >> > wrote: >> > >> > A commit message would be nice. A brief rundown of how this is called >> > from userspace would be nice (I'm talking a single sentence here, e.g. >> > "this is allocated when submitting a nfc packet") and what issue >> > __GFP_NOWARN is fixing. (I'm guessing log spam due to allocation >> > failures.) >> > >> >> I thought it is obvious. ;) Keep in mind that $subject is one part of commit >> message too, so there is a commit message although very short. >> >> I will add it. > > > BTW, kzalloc() is useless here, since it is followed by > > if (memcpy_from_msg(msg_data, msg, len)) { Hmm? I think nfc_llcp_send_ui_frame() needs to do some fragmention with this temporary memory, or you are saying msg_iter has some API available to seek the pointer? Even if so, it doesn't look like suitable for -stable. > > Also, this file seems to have two spots with the same problem, > in nfc_llcp_send_ui_frame() & nfc_llcp_send_i_frame() > Ah, yes, I missed nfc_llcp_send_i_frame().
Re: [PATCH RESEND net] nfc: use GFP_USER for user-controlled kmalloc
On Wed, 2016-01-27 at 11:50 -0800, Cong Wang wrote: > Hmm? I think nfc_llcp_send_ui_frame() needs to do some fragmention > with this temporary memory, or you are saying msg_iter has some > API available to seek the pointer? Even if so, it doesn't look like > suitable for -stable. > memcpy_from_msg(msg_data, msg, len) will overwrite the msg_data with len bytes, or return an error. So prior msg_data content does not matter. kzalloc() before a memset() or memcpy() sounds defensive programming, kmalloc() is a bit faster.
Re: [PATCH RESEND net] nfc: use GFP_USER for user-controlled kmalloc
Hi Cong, On Wed, Jan 27, 2016 at 4:53 AM, Cong Wangwrote: A commit message would be nice. A brief rundown of how this is called from userspace would be nice (I'm talking a single sentence here, e.g. "this is allocated when submitting a nfc packet") and what issue __GFP_NOWARN is fixing. (I'm guessing log spam due to allocation failures.) > Reported-by: Dmitry Vyukov > Cc: Lauro Ramos Venancio > Cc: Aloisio Almeida Jr > Cc: Samuel Ortiz > Signed-off-by: Cong Wang > --- > net/nfc/llcp_commands.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/nfc/llcp_commands.c b/net/nfc/llcp_commands.c > index 3621a90..5d94055 100644 > --- a/net/nfc/llcp_commands.c > +++ b/net/nfc/llcp_commands.c > @@ -729,7 +729,7 @@ int nfc_llcp_send_ui_frame(struct nfc_llcp_sock *sock, u8 > ssap, u8 dsap, > if (local == NULL) > return -ENODEV; > > - msg_data = kzalloc(len, GFP_KERNEL); > + msg_data = kzalloc(len, GFP_USER | __GFP_NOWARN); > if (msg_data == NULL) > return -ENOMEM; Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH RESEND net] nfc: use GFP_USER for user-controlled kmalloc
On Tue, Jan 26, 2016 at 2:55 PM, Julian Calabywrote: > Hi Cong, > > On Wed, Jan 27, 2016 at 4:53 AM, Cong Wang wrote: > > A commit message would be nice. A brief rundown of how this is called > from userspace would be nice (I'm talking a single sentence here, e.g. > "this is allocated when submitting a nfc packet") and what issue > __GFP_NOWARN is fixing. (I'm guessing log spam due to allocation > failures.) > I thought it is obvious. ;) Keep in mind that $subject is one part of commit message too, so there is a commit message although very short. I will add it.
Re: [PATCH RESEND net] nfc: use GFP_USER for user-controlled kmalloc
Hi Cong, On Wed, Jan 27, 2016 at 10:12 AM, Cong Wangwrote: > On Tue, Jan 26, 2016 at 2:55 PM, Julian Calaby > wrote: >> Hi Cong, >> >> On Wed, Jan 27, 2016 at 4:53 AM, Cong Wang wrote: >> >> A commit message would be nice. A brief rundown of how this is called >> from userspace would be nice (I'm talking a single sentence here, e.g. >> "this is allocated when submitting a nfc packet") and what issue >> __GFP_NOWARN is fixing. (I'm guessing log spam due to allocation >> failures.) >> > > I thought it is obvious. ;) Keep in mind that $subject is one part of commit > message too, so there is a commit message although very short. > > I will add it. I know almost nothing about how the NFC subsystem works, and this looks like a potential security issue, so more information is better IMHO. Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH RESEND net] nfc: use GFP_USER for user-controlled kmalloc
On Tue, 2016-01-26 at 15:12 -0800, Cong Wang wrote: > On Tue, Jan 26, 2016 at 2:55 PM, Julian Calaby> wrote: > > Hi Cong, > > > > On Wed, Jan 27, 2016 at 4:53 AM, Cong Wang wrote: > > > > A commit message would be nice. A brief rundown of how this is called > > from userspace would be nice (I'm talking a single sentence here, e.g. > > "this is allocated when submitting a nfc packet") and what issue > > __GFP_NOWARN is fixing. (I'm guessing log spam due to allocation > > failures.) > > > > I thought it is obvious. ;) Keep in mind that $subject is one part of commit > message too, so there is a commit message although very short. > > I will add it. BTW, kzalloc() is useless here, since it is followed by if (memcpy_from_msg(msg_data, msg, len)) { Also, this file seems to have two spots with the same problem, in nfc_llcp_send_ui_frame() & nfc_llcp_send_i_frame()