On Tue, Apr 23, 2024 at 09:46:35PM +0700, Bui Quang Minh wrote: > > > - buffer = vmemdup_user(buf, lbuf); > > > + buffer = vmemdup_user(buf, lbuf + 1); > > > if (IS_ERR(buffer)) > > > return -ENOMEM; > > > + buffer[lbuf] = '\0'; > > > > This would read one byte too much from user space, and could potentially > > fault. > > > > Why isn't this simply memdup_user_nul() like all others, which would do the > > right thing? ... > For this case, as the original code uses vmemdup_user, which internally uses > kvmalloc not kmalloc, so I try to keep the original behavior. And > vmemdup_user does not have the counterpart vmemdup_user_nul. I can > kvmalloc(lbuf + 1), then copy_to_user(lbuf) and set buffer[lbuf] = '\0' or > do you think I should create vmemdup_user_nul? There is no need for vmalloc() instead of kmalloc() for this particular case. The input string is supposed to be rather short (see the sscanf() call). So converting to memdup_user_nul() is sufficient and solves the potential problem.
